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

Bump go-discover to fix broken dep tree #12404

Merged
merged 1 commit into from Aug 24, 2021
Merged

Bump go-discover to fix broken dep tree #12404

merged 1 commit into from Aug 24, 2021

Conversation

jeffwidman
Copy link
Contributor

The previous version of go-discover pulled in a broken version of
tencentcloud-sdk-go, resulting in anything that runs go get -d
downstream breaking... ie, a dep on hashicorp vault will break
Dependabot (among other things).

I already fixed it in go-discover, so this just pulls in the update.

More details in
hashicorp/go-discover@657e803
and hashicorp/go-discover#172.

The previous version of `go-discover` pulled in a broken version of
`tencentcloud-sdk-go`, resulting in anything that runs `go get -d`
downstream breaking... ie, a dep on hashicorp vault will break
Dependabot (among other things).

I already fixed it in `go-discover`, so this just pulls in the update.

More details in
hashicorp/go-discover@657e803
and hashicorp/go-discover#172.
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 23, 2021 18:31 Inactive
@vercel vercel bot temporarily deployed to Preview – vault August 23, 2021 18:31 Inactive
@jeffwidman
Copy link
Contributor Author

This should be an extremely low-risk upgrade. The only thing that changed in go-discover were this dep bump and a cosmetic fix to the repo's CI badge on the readme to point at Circle-CI instead of travis.

@jeffwidman jeffwidman changed the title Bump go-discover to fix broken dep Bump go-discover to fix broken dep tree Aug 23, 2021
@jeffwidman
Copy link
Contributor Author

jeffwidman commented Aug 23, 2021

Test failure https://app.circleci.com/pipelines/github/hashicorp/vault/20115/workflows/8c250796-0afd-41df-a075-c7d42e0a0730/jobs/269466 appears unrelated... looks like flaky test infra unable to connect to Cassandra.

@dtomcej
Copy link

dtomcej commented Aug 24, 2021

Can the CI be rerun to unblock this PR?

Ideally this would be merged before 1.8.2 is released:
#12407

@ncabatoff ncabatoff merged commit ce442ad into hashicorp:main Aug 24, 2021
@ncabatoff
Copy link
Collaborator

Thanks @jeffwidman . @dtomcej , why do you want this change in 1.8.2?

@jeffwidman
Copy link
Contributor Author

jeffwidman commented Aug 24, 2021

Thanks for merging!

why do you want this change in 1.8.2?

Currently anywhere we havevault in the dep tree of our repos it will break go tooling that walks the dep tree like go get -d etc because it tries to find the underlying tencentcloud-sdk-go version which has been completely removed from GitHub. This may be improved with go 1.17's lazy module loading, I'm not sure as we haven't upgraded yet. Additionally, any tooling that uses this go tooling will break as well. For my scenario in particular, suddenly Dependabot stopped working across 20+ repos.

I realize we can work around it with a replace directive, but that's a pain to maintain because we have to add it and then remember to remove the replace line once it's fixed... much simpler for us to simply get the proper fix in here upstream.

@jeffwidman jeffwidman deleted the bump-go-discover-past-broken-version branch August 24, 2021 16:38
@ncabatoff
Copy link
Collaborator

Thanks for clarifying. I'm afraid it's too late to squeeze it into 1.8.2, the release branch is frozen for all non-essential changes. I'm not sure what you're depending on hashicorp/vault (the module) for, but we generally discourage that, asking people to depend on the sdk and api sub-modules instead. I understand you may have your reasons, I'm not telling you mustn't, just telling you that we don't provide much in the way of guarantees if you do so.

@jeffwidman
Copy link
Contributor Author

jeffwidman commented Aug 24, 2021

I'm afraid it's too late to squeeze it into 1.8.2, the release branch is frozen for all non-essential changes.

No problem, I just thought it was already in 1.8.2 since this had been merged before before the tag cut, didn't realize you had a separate release branch. Waiting another few weeks is fine as long as it's in on a release train at some point in the future.

I'm not sure what you're depending on hashicorp/vault (the module) for, but we generally discourage that, asking people to depend on the sdk and api sub-modules instead. I understand you may have your reasons, I'm not telling you mustn't, just telling you that we don't provide much in the way of guarantees if you do so.

Yep, I looked into this a while back. But we couldn't find any vault.NewTestCluster() equivalent in api/sdk... Has that changed at all?

jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
The previous version of `go-discover` pulled in a broken version of
`tencentcloud-sdk-go`, resulting in anything that runs `go get -d`
downstream breaking... ie, a dep on hashicorp vault will break
Dependabot (among other things).

I already fixed it in `go-discover`, so this just pulls in the update.

More details in
hashicorp/go-discover@657e803
and hashicorp/go-discover#172.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants