Better error handling when SHASUM file is missing #1439
Conversation
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
|
This PR seems to have commented-out code, moving back to draft. |
Signed-off-by: Diogenes Fernandes <[email protected]>
There was a problem hiding this comment.
I'm not sure if it's possible, but it would be nice if this functionality reported back as a comment to the submitter because it would make debugging issues easier. Then again, most of this repository would need to be moved over to libregistry anyway, so maybe we can do it as part of that move.
| @@ -28,7 +28,7 @@ func main() { | |||
| logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) | |||
|
|
|||
| repository := flag.String("repository", "", "The provider repository to add") | |||
| outputFile := flag.String("output", "", "Path to write JSON result to") | |||
| outputFile := flag.String("output", "output.json", "Path to write JSON result to") | |||
There was a problem hiding this comment.
It's just because if you don't specify the output flag, the program panics next to the JSON function that writes to this output.
There was a problem hiding this comment.
So I'm adding the default in case someone doesn't pass the flag
There was a problem hiding this comment.
I'd rather the program exit early if the flag is not provided
| @@ -28,7 +28,7 @@ func main() { | |||
| logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) | |||
|
|
|||
| repository := flag.String("repository", "", "The provider repository to add") | |||
| outputFile := flag.String("output", "", "Path to write JSON result to") | |||
| outputFile := flag.String("output", "output.json", "Path to write JSON result to") | |||
There was a problem hiding this comment.
It's just because if you don't specify the output flag, the program panics next to the JSON function that writes to this output.
|
Your review makes sense @abstractionfactory ! I will work later to port this code to libregistry, and add the code to report back as a comment in the registry repo. I'll leave this PR open for now for changing the Github actions to report the comment. |
|
@diofeher I think we can skip the libregistry part for now, we still need to figure out what our roadmap is to get that going. However, I'd still like to get this merged before that. |
| @@ -13,7 +13,7 @@ func (p Provider) GetSHASums(shaFileDownloadUrl string) (map[string]string, erro | |||
| return nil, fmt.Errorf("failed to download asset contents: %w", assetErr) | |||
| } | |||
| if contents == nil { | |||
| return nil, nil | |||
| return nil, fmt.Errorf("SHASUM file is empty or missing: %s", shaFileDownloadUrl) | |||
There was a problem hiding this comment.
There was a very specific reason I didn't return an error here. We don't want new releases (with missing shasums) to cause a whole provider to stop updating. A lot of providers have historical releases that we scan from time to time which would percolate this error back up the stack.
We either need to add a flag to the calls tack to make this a true error or not, or create a specific error type that is handled correctly.
There was a problem hiding this comment.
Makes sense @cam72cam
Should we put what your explained to me in this part of the code as well? I feel this is important information that should be in a comment
There was a problem hiding this comment.
So this seems to be more complicated than I thought, because all workflows are using this function, so we would need to identify what exact behavior we need when adding a provider from Github Actions or from other places. I'm gonna do a research later on this.
There was a problem hiding this comment.
A comment is a great idea, a lot of this was hastily adapted from prototype so context may be missing for quirks like this.
Closes #280
Description
Tested with https://github.com/diofeher/terraform-provider-nix-signature/releases/tag/v0.1.1