Use regex for release asset download #50

coryvirok · 2019-02-18T23:54:56Z

Caveat: The checksum functionality for verifying release assets still only supports 1 asset. If multiple assets match the regex provided, an error will be returned.

Multiple checksums can now be passed via the command line and the downloaded assets must match one of them.

… multiple release assets at once.

yorinasub17

Thanks for your contribution! There are a few things I commented on but the overall approach seems reasonable!

I also have a few general follow up questions and requests:

Can you update the docs, both the CLI option text and the README, to reflect that --release-asset can be a regex?
Can you add a test case that tests the regex and multiple downloads code path? You should be able to craft a regex that matches multiple assets on the sample repo we use for testing.
If you don't mind, can you share the use case that prompted this? This is more out of curiosity than anything.

yorinasub17 · 2019-02-19T02:57:43Z

main.go

+func findAssetsInRelease(assetRegex string, release GitHubReleaseApiResponse) ([](*GitHubReleaseAsset), error) {
+	var matches [](*GitHubReleaseAsset)
+
+	pattern := regexp.MustCompile(assetRegex)


Since the Regex is user provided, we should use Compile and bubble the error in a friendly message (e.g Could not parse provided release asset regex: ERROR) to the user indicating that the regex is unparsable as opposed to panicking.

yorinasub17 · 2019-02-19T03:18:48Z

main.go

 }

 // Download the specified binary file that was uploaded as a release asset to the specified GitHub release.
 // Returns the path where the release asset was downloaded.


Can you update this comment so that it captures the fact that we now:

match the regex against all the assets on the release tag

use go routines to download each one.

yorinasub17 · 2019-02-19T03:20:12Z

main.go

+				errorStrs = append(errorStrs, downloadErr.Error())
+			}
+
+			wg.Done()


NIT: in go, it is idiomatic to "schedule" clean up functions at the start (using defer). That way, it is easier to ensure all early exits are covered. Right now this function is simple, but over time this could get more complex so having it at the top as the first thing is good practice.

main.go

yorinasub17 · 2019-02-19T03:25:24Z

main.go

+			assetPath := path.Join(destPath, asset.Name)
+			fmt.Printf("Downloading release asset %s to %s\n", asset.Name, assetPath)
+			if downloadErr := DownloadReleaseAsset(githubRepo, asset.Id, assetPath); downloadErr == nil {
+				assetPaths = append(assetPaths, assetPath)


IIRC, append is not goroutine safe. Can you either use a mutex to guard this section, or use channels to collect the results?

yorinasub17 · 2019-02-19T03:26:39Z

main.go

-		fetchErr = verifyChecksumOfReleaseAsset(assetPath, options.ReleaseAssetChecksum, options.ReleaseAssetChecksumAlgo)
+	if options.ReleaseAssetChecksum != "" && len(assetPaths) > 0 {
+		// TODO: Check more than just the first checksum if multiple assets were downloaded
+		fetchErr = verifyChecksumOfReleaseAsset(assetPaths[0], options.ReleaseAssetChecksum, options.ReleaseAssetChecksumAlgo)


This behavior is a bit unintuitive. I would feel more comfortable if we error out the checksum check saying we don't support checksum checks if there are more than 1 assets matching the regex. However, I could use a second opinion on this. What do you think @josh-padnick ?

I had the same thought. If the RegEx matches more than one asset, it seems like matching arbitrarily on the first of those matches is likely to lead to confusing behavior. If we really do have more than one asset path match the RegEx, I'd rather we return an error message with exactly what @yorinasub17 suggested: "Error: The regular expression returned more than one match, and fetch only supports Regular Expressions that have exactly 1 one match."

I forget if fetch has any concept of debug logging, but as an added bonus, it'd be nice to show the user all the matches when in debug mode.

@coryvirok actually, @josh-padnick and I talked again and we realized that maybe we can support checksum checks by supporting passing in multiple checksums via --release-asset-checksum and if the asset matches any of them, we pass the check.

If you can make the change to support this, that would be great! This should be possible (and backwards compatible) by using StringSliceFlag instead of StringFlag for --release-asset-checksum, which then allows passing in multiple entries. Then, you can use our ListContainsElement https://github.com/gruntwork-io/gruntwork-cli/blob/master/collections/lists.go#L4 function to check if the checksum list contains the asset checksum.

yorinasub17 · 2019-02-19T03:28:36Z

main.go

 	fmt.Println("Download of release assets complete.")
-	return assetPath, nil
+	if numErrors := len(errorStrs); numErrors > 0 {
+		err = fmt.Errorf("%d errors while downloading assets:\n%s", numErrors, strings.Join(errorStrs, "\n\tError: "))


NIT: Is there a way to provide the asset name with the corresponding error message here?

- Added a test for downloading multiple assets - Changed the checksum flag implementation to error if more than 1 asset matches the regex - Updated the README with an example that uses a regex

coryvirok · 2019-02-19T05:40:06Z

Thanks for the feedback, @yorinasub17 and @josh-padnick. I appreciate the code review and nits since I'm very new to Golang development. Please let me know if I've missed anything.

To answer your question regarding the motivation for this change:

I was looking for a tool that would allow me to easily download assets uploaded to a GitHub release using a personal auth token. I found your tool and liked it, however the assets uploaded to the releases I'm interested in have version numbers in their names, e.g. "foo-1.2.3-linux_amd64". This made it difficult to download the "latest version" of the release asset since it would require that I provide the version number in the requested asset name. So, I figured it was easier and more productive to modify your tool than write my own unmaintainable bash scripts. I also saw a similar issue opened and someone mentioned that you'd accept PRs for this functionality. (#29) So, hopefully this helps out those folks too.

yorinasub17

Thanks for the updates! Have a few more suggestions, but this is getting closer!

yorinasub17 · 2019-02-19T06:12:15Z

main_test.go

+
+	if len(assetPaths) != 2 {
+		t.Fatalf("Expected to download 2 assets, not %d", len(assetPaths))
+	}


Can you also add a check to make sure the files exists in tmpDir? Here is how you can do this in go: https://stackoverflow.com/a/12518877

yorinasub17 · 2019-02-19T06:12:58Z

main_test.go

@@ -0,0 +1,50 @@
+package main


Can you also add one more test to make sure we test the error code path for the checksum?

yorinasub17 · 2019-02-19T06:17:56Z

main.go

+				fmt.Printf("Downloaded %s\n", assetPath)
+				results <- Pair{assetPath, nil}
+			} else {
+				msg := downloadErr.Error()


NIT: No need to do this. You can simply use downloadErr where you use msg.

yorinasub17 · 2019-02-19T06:20:43Z

main.go


+type Pair struct {
+	a, b interface{}
+}


Since we aren't using this outside of downloadReleaseAssets, can we rename this to AssetDownloadResult and use proper types? (string and error). That way, we can avoid the type conversions.

yorinasub17 · 2019-02-19T06:25:42Z

README.md

 ```

-Download the release asset `foo.exe` from release `0.1.5` and save it to `/tmp`:
+Download all release assets matching the regular expression, `foo_linux-.*` from release `0.1.5` and save them to `/tmp`:


NIT: Can you keep the original example, and add this as another example? It is a simple example and is a nice way of showing users that you can pass in the direct name to get the single asset.

yorinasub17 · 2019-02-19T17:37:09Z

main.go

-		fetchErr = verifyChecksumOfReleaseAsset(assetPath, options.ReleaseAssetChecksum, options.ReleaseAssetChecksumAlgo)
+	if options.ReleaseAssetChecksum != "" && len(assetPaths) > 0 {
+		// TODO: Check more than just the first checksum if multiple assets were downloaded
+		fetchErr = verifyChecksumOfReleaseAsset(assetPaths[0], options.ReleaseAssetChecksum, options.ReleaseAssetChecksumAlgo)


@coryvirok actually, @josh-padnick and I talked again and we realized that maybe we can support checksum checks by supporting passing in multiple checksums via --release-asset-checksum and if the asset matches any of them, we pass the check.

If you can make the change to support this, that would be great! This should be possible (and backwards compatible) by using StringSliceFlag instead of StringFlag for --release-asset-checksum, which then allows passing in multiple entries. Then, you can use our ListContainsElement https://github.com/gruntwork-io/gruntwork-cli/blob/master/collections/lists.go#L4 function to check if the checksum list contains the asset checksum.

- Added the old example back into the README along with the new one - Implemented multiple checksum flag - Added tests for multiple checksum pass/fail conditions - Misc tweaks from feedback

yorinasub17 · 2019-02-20T20:51:26Z

@coryvirok Ok I think this is ready to merge with your latest changes. I am going to go ahead and merge this in, and let our builds run. If it passes I'll cut a new release. Thanks for your contribution!

yorinasub17 · 2019-02-20T20:56:22Z

https://github.com/gruntwork-io/fetch/releases/tag/v0.3.3

coryvirok · 2019-02-21T02:41:56Z

Fantastic, thanks!

Changed the release asset download to accept a regex name to download…

50ffe04

… multiple release assets at once.

coryvirok requested review from josh-padnick, tonerdo and yorinasub17 as code owners February 18, 2019 23:54

Fix pointer reference and remove debugging

586ee92

yorinasub17 reviewed Feb 19, 2019

View reviewed changes

yorinasub17 self-assigned this Feb 19, 2019

Implement code review feedback

c26f0de

- Added a test for downloading multiple assets - Changed the checksum flag implementation to error if more than 1 asset matches the regex - Updated the README with an example that uses a regex

yorinasub17 reviewed Feb 19, 2019

View reviewed changes

More code review feedback

4e2af97

- Added the old example back into the README along with the new one - Implemented multiple checksum flag - Added tests for multiple checksum pass/fail conditions - Misc tweaks from feedback

yorinasub17 merged commit 1c09f1a into gruntwork-io:master Feb 20, 2019

Use regex for release asset download #50

Use regex for release asset download #50

Uh oh!

Conversation

coryvirok commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yorinasub17 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryvirok commented Feb 19, 2019

Uh oh!

yorinasub17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorinasub17 commented Feb 20, 2019

Uh oh!

yorinasub17 commented Feb 20, 2019

Uh oh!

coryvirok commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coryvirok commented Feb 18, 2019 •

edited

Loading

yorinasub17 left a comment •

edited

Loading