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

Update go-getter to v1.5.1 with support for vhost style S3 paths #9349

Merged
merged 4 commits into from
Jan 7, 2021

Conversation

picatz
Copy link
Contributor

@picatz picatz commented Nov 13, 2020

Aims to fix #9050 and related to hashicorp/go-getter#263

Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

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

LGTM! sorry this sat for so long.

@picatz, does this resolve #8454, then?

@cgbaker
Copy link
Contributor

cgbaker commented Dec 7, 2020

note, i'm seeing the same checksum error that was reported in lint-go. maybe a go version mismatch?

scratch that, i'm seeing a different complaint, looking into it now.

@picatz
Copy link
Contributor Author

picatz commented Dec 7, 2020

Yup, this fix should also resolve #8454 👍

@cgbaker cgbaker mentioned this pull request Dec 7, 2020
@schmichael
Copy link
Member

Is this mergable? I don't quite understand what the lint failure is:

-github.com/hashicorp/go-getter v1.5.0 h1:ciWJaeZWSMbc5OiLMpKp40MKFPqO44i0h3uyfXPBkkk=
-github.com/hashicorp/go-getter v1.5.0/go.mod h1:a7z7NPPfNQpJWcn4rSWFtdrSldqLdLPEF3d8nFMsSLM=

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 6, 2021 13:17 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad January 6, 2021 13:17 Inactive
@tgross
Copy link
Member

tgross commented Jan 6, 2021

Is this mergable? I don't quite understand what the lint failure is:

That's because we needed to run make sync. Unfortunately on top of that the go-getter 1.5.1 tag has changed from when this PR was created! That seems like a miscommunication in what was supposed to be tagged, but see https://github.com/hashicorp/go-getter/commits/v1.5.1 for the current state of this tag.

I've rebased this PR on master and updated the go.sum to match.

@tgross
Copy link
Member

tgross commented Jan 6, 2021

Well now the linter is getting the same error I got locally when I tried to make sync, except with the old checksum:

go: downloading github.com/hashicorp/go-getter v1.5.1
verifying github.com/hashicorp/go-getter@v1.5.1: checksum mismatch
	downloaded: h1:lM9sM02nvEApQGFgkXxWbhfqtyN+AyhQmi+MaMdBDOI=
	go.sum:     h1:LZ49OxqBBtdKJymlpX7oTyqGBQRg4xxQDyPW4hzoZqM=

SECURITY ERROR
This download does NOT match an earlier download recorded in go.sum.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 6, 2021 13:47 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad January 6, 2021 13:47 Inactive
@tgross
Copy link
Member

tgross commented Jan 6, 2021

So as far as I can tell the make sync error I was getting is operating system dependent. On macOS go1.15.6 I get a different checksum than I do on Linux go1.15.6. I've pushed up the fix to match the checksums and that should fix the linter issue... I'll debug the checksum issue separately.

Edit: looks like it's a go module proxy problem. Using GOPROXY=direct on Linux makes it match with what I get on macOS.

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 7, 2021 18:02 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad January 7, 2021 18:02 Inactive
@tgross
Copy link
Member

tgross commented Jan 7, 2021

Ok, following an internal discussion it looks like this was down to a release error in go-getter for the 1.5.1 tag. I've bumped go-getter to 1.5.2 and this should be safe to merge after that.

@tgross tgross merged commit e1e7303 into master Jan 7, 2021
@tgross tgross deleted the update-go-getter branch January 7, 2021 18:34
@tgross tgross added this to the 1.0.2 milestone Jan 7, 2021
@picatz
Copy link
Contributor Author

picatz commented Jan 7, 2021

Thank you everyone that helped get this debugged / merged! 🙇

@github-actions
Copy link

github-actions bot commented Dec 4, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Artifact stanza doesn't support AWS S3 virtual host style paths
5 participants