Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output release asset contents to standard output #103

Merged
merged 5 commits into from
Jan 29, 2022
Merged

Conversation

tonerdo
Copy link
Contributor

@tonerdo tonerdo commented Jan 24, 2022

Description

An initial stab at supporting sending the contents of a downloaded asset to standard output

Documentation

TODOs

Please ensure all of these TODOs are completed before asking for a review.

  • Ensure the branch is named correctly with the issue number. e.g: feature/new-vpc-endpoints-955 or bug/missing-count-param-434.
  • Update the docs.
  • Keep the changes backward compatible where possible.
  • Run the pre-commit checks successfully.
  • Run the relevant tests successfully.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.

Related Issues

Addresses #101

robmorgan
robmorgan previously approved these changes Jan 26, 2022
Copy link
Contributor

@robmorgan robmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonerdo LGTM. do you think we need test cases to check the behaviour for a single asset vs multiple assets?

@tonerdo
Copy link
Contributor Author

tonerdo commented Jan 26, 2022

@robmorgan I agree on test cases and I spent some time thinking through them.. Any idea how to mock a cli.Context? That's my blocker.

@robmorgan
Copy link
Contributor

@tonerdo you could try to create an integration test in integration_test.go with something like:

func TestFetchWithStdoutOption(t *testing.T) {
	tmpDownloadPath, err := ioutil.TempDir("", "fetch-stdout-test")
	require.NoError(t, err)

	repoUrl := "https://github.com/gruntwork-io/fetch-test-public"
	releaseTag := "v0.0.4"
	releaseAsset := "hello+world.txt"

	cmd := fmt.Sprintf("fetch --repo %s --tag %s --release-asset %s --stdout false %s", repoUrl, releaseTag, releaseAsset, tmpDownloadPath)
	t.Logf("Testing command: %s", cmd)
	stdoutput, _, err := runFetchCommandWithOutput(t, cmd)
	require.NoError(t, err)

	// Ensure the expected file was downloaded
	assert.FileExists(t, JoinPath(tmpDownloadPath, releaseAsset))

	// When --stdout is specified, ensure the file contents are piped to the standard output stream
	assert.Contains(t, stdoutput, "hello world")
}

Maybe if you do a loop with 2 test cases it might cover each code path?

@tonerdo
Copy link
Contributor Author

tonerdo commented Jan 28, 2022

Ah! Didn't realize we had integration tests. Thanks for the pointer

Copy link
Contributor

@robmorgan robmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok LGTM

@tonerdo tonerdo merged commit cdd305d into master Jan 29, 2022
@tonerdo tonerdo mentioned this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants