Closed pete0emerson closed 3 years ago
Hey @pete0emerson,
Thanks for submitting a fix so quickly! I tested your logic locally and I can confirm it works with my use case. In regards to your comments around testing, I think the right way to solve this is with integration tests! That way we can easily assert our intended behavior from a 9000ft view and not in all of these individual functions. I've gone ahead and added one specifically for branches to this PR (more info below), however, since I'm now a contributor to this PR, we'll need to get @brikis98 to review.
Side note: with your latest change, I think we might need to adjust the help text here https://github.com/gruntwork-io/fetch/blob/master/main.go#L69 as I believe git references will no longer override commits, tags or branches. However, I may have interpreted that incorrectly?
Whilst inspired by the integration tests in Terragrunt, this was slightly non-trivial for fetch
as we write directly to os.Stdout()
and os.Stderr()
. Therefore I had to do some voodoo magic in runFetchCommandWithOutput
to temporarily override these pipes.
Feel free to let me know what you think.
Roger!
On Tue, Feb 2, 2021 at 12:40 PM Rob Morgan notifications@github.com wrote:
@robmorgan commented on this pull request.
In integration_test.go https://github.com/gruntwork-io/fetch/pull/88#discussion_r568570079:
+
- // Private repo equivalent
- {"branch option with private repo", "https://github.com/gruntwork-io/fetch-test-private", "sample-branch", "/"},
- }
- for _, tc := range cases {
- tc := tc
- t.Run(tc.name, func(t *testing.T) {
- cmd := fmt.Sprintf("fetch --repo %s --branch %s --source-path %s %s", tc.repoUrl, tc.branchName, tc.sourcePath, tmpDownloadPath)
- //t.Fatal(cmd)
- output, _, err := runFetchCommandWithOutput(t, cmd)
- require.NoError(t, err)
- // When --branch is specified, ensure the latest commit is fetched
- assert.Contains(t, output, "Downloading latest commit from branch")
Yes, I think we're in alignment now. I'm not sure if you missed the fact I mentioned a logger above:
Because we can continue to use the new integration helper methods like runFetchCommandWithOutput until we eventually move our fetch code over to use an inbuilt logger.
I'm under the opinion that this is non-ideal, but moving in the right direction, so I'm going to leave it as is, but improve the test cases and mention why I'm not running the tests in parallel. Eventually, we can introduce a logger.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gruntwork-io/fetch/pull/88#discussion_r568570079, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNZZEIAHMAXRDLOJCK4ZDS47XDXANCNFSM4W5KWQ2A .
New version released in https://github.com/gruntwork-io/fetch/releases/tag/v0.3.14
This closes #87.
This has no additional tests yet.
We don't have specific tests for
MakeGitHubZipFileRequest
, which I modified to put the conditional for GitRef last.We do test
downloadGithubZipFile
which callsMakeGitHubZipFileRequest
, and it seems like our testTestDownloadGitBranchZipFile
would be designed to catch this. Here's the logic that I unwrapped for this particular case:I'm open to suggestions on improving testing here, it seems like
downloadGithubZipFile
would need to be tested with multiple things set to make sure it does the right action, but I'm not positive.Here's the evidence of this fixing the problem @brikis98 encountered. I also ran all of the tests, and they're passing.