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.

Sign up for GitHub

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

Jump to bottom

Vendor new Vault API package #4658

Merged
merged 6 commits into from
Sep 11, 2018
Merged

Vendor new Vault API package #4658

merged 6 commits into from
Sep 11, 2018

Conversation

dadgar
Copy link
Contributor

@dadgar dadgar commented Sep 10, 2018

Fixes #4633

schmichael
schmichael reviewed Sep 11, 2018
View reviewed changes
vendor/vendor.json Outdated
@@ -196,11 +196,12 @@
{"path":"github.com/hashicorp/serf/coordinate","checksumSHA1":"0PeWsO2aI+2PgVYlYlDPKfzCLEQ=","revision":"80ab48778deee28e4ea2dc4ef1ebb2c5f4063996","revisionTime":"2018-05-07T23:19:28Z"},
{"path":"github.com/hashicorp/serf/serf","checksumSHA1":"QrT+nzyXsD/MmhTjjhcPdnALZ1I=","revision":"80ab48778deee28e4ea2dc4ef1ebb2c5f4063996","revisionTime":"2018-05-07T23:19:28Z"},
{"path":"github.com/hashicorp/vault","checksumSHA1":"eGzvBRMFD6ZB3A6uO750np7Om/E=","revision":"182ba68a9589d4cef95234134aaa498a686e3de3","revisionTime":"2016-08-21T23:40:57Z"},
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the root pkg matching as well?

Copy link
Member

Choose a reason for hiding this comment

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

Actually it's a main pkg... Probably shouldn't vendor it at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I removed the top level from vendor

preetapan
preetapan approved these changes Sep 11, 2018
View reviewed changes
Copy link
Member

@preetapan preetapan left a comment

Choose a reason for hiding this comment

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

LGTM, could you add a comment explaining why circonus needed to be revendored

@dadgar
Copy link
Contributor Author

dadgar commented Sep 11, 2018

@preetapan circonus was using an older version of our retryable http client and the vendored vault api was relying on the new one. So there was a build error unless I vendored circonus such that both were using the new API

@dadgar dadgar merged commit 83858a8 into master Sep 11, 2018
@dadgar dadgar deleted the b-vault-panic branch September 11, 2018 19:12
@kak-tus kak-tus mentioned this pull request Sep 13, 2018
Closed
@schmichael schmichael mentioned this pull request Sep 26, 2018
Closed
@github-actions
Copy link

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 Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schmichael schmichael schmichael approved these changes

@preetapan preetapan preetapan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dadgar @schmichael @preetapan