Skip to content

Conversation

azr
Copy link
Contributor

@azr azr commented Mar 16, 2021

After #310, @ScottSuarez proposed we put the GCP generational content in our bucket so that test things are in one place.
This PR uses our bucket.

Copy link
Contributor

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

lgtm :)

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Just one remaining question - Is the access to the bucket documented somewhere internally? No need to discuss where or how (here on GitHub), just checking to make sure it is.

t.Fatalf("err: %s", err)
}
if string(content) != "a" {
if string(content) != "a\n" {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're adding the newlines to the test data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I created the file using echo a > file.txt and it added a \n, and I just felt like this was an okay change.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair. Trailing newline seems like a good practice - I just didn't know if there's anything more to it 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing !

@azr azr merged commit 7b99c31 into main Apr 1, 2021
@azr azr deleted the azr_use_our_test_bucket branch April 1, 2021 08:46
ebudan pushed a commit to ebudan/go-getter that referenced this pull request Jul 22, 2021
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.

3 participants