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

Add PutAutoPilotRaftConfiguration to api #12428

Merged
merged 1 commit into from Nov 10, 2021
Merged

Add PutAutoPilotRaftConfiguration to api #12428

merged 1 commit into from Nov 10, 2021

Conversation

elsesiy
Copy link
Contributor

@elsesiy elsesiy commented Aug 25, 2021

The previous implementation only allows retrieving the autopilot state but not modifying it, hence the missing method has been added.

Misc:

  • Update api to go1.16
  • Update dependencies

Fix #12427

@vercel vercel bot temporarily deployed to Preview – vault August 25, 2021 05:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 25, 2021 05:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault August 25, 2021 05:54 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 25, 2021 05:54 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 25, 2021 05:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault August 25, 2021 05:58 Inactive
@elsesiy
Copy link
Contributor Author

elsesiy commented Aug 27, 2021

@calvn @ncabatoff Could you please take a look? Thanks!

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

The new method looks fine. Can you revert the other bits? We can't rename public methods without incrementing major version, since that's a breaking change. I don't want think that's worth it at this point. Also, I don't see any reason to include module updates (changes to go.mod/go.sum) in this PR.

@hsimon-hashicorp
Copy link
Contributor

Hi @elsesiy - if you can take a look at @ncabatoff's comment and update and let us know you've done so, we'll get this reviewed and merged. Thanks so much!

The previous implementation only allows retrieving the autopilot state but not modifying it, hence the missing method has been added

Fix #12427
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 7, 2021 20:59 Inactive
@vercel vercel bot temporarily deployed to Preview – vault September 7, 2021 20:59 Inactive
@elsesiy
Copy link
Contributor Author

elsesiy commented Sep 7, 2021

@hsimon-hashicorp @ncabatoff I updated the PR according to the comments. Should I submit a separate one for the go toolchain/module changes?

@ncabatoff
Copy link
Collaborator

@hsimon-hashicorp @ncabatoff I updated the PR according to the comments. Should I submit a separate one for the go toolchain/module changes?

Sorry, I've already forgotten what they were. :)

Going by

Update api to go1.16
Update dependencies

my hunch is no, since we don't want the api to impose go 1.16 (unless you know of a reason why it should - I don't, but I may be missing something) and we don't update dependencies automatically just because they're out of date. But feel free to open a PR for them if you'd like us to take a closer look.

@ncabatoff ncabatoff added this to the 1.9 milestone Sep 7, 2021
@elsesiy
Copy link
Contributor Author

elsesiy commented Nov 7, 2021

@ncabatoff When will this be merged? I see it's tagged for 1.9 but it didn't make it into the rc..

@ncabatoff ncabatoff modified the milestones: 1.9, 1.9.1 Nov 10, 2021
@ncabatoff
Copy link
Collaborator

@elsesiy sorry about that, stuff came up, and CI is stuck and not running the tests. I don't know of a good way of getting CI unstuck when this happens... the only thing I've found that works in the past sometimes is to add a new commit. In this case the change is purely additive, so I decided to test that it doesn't break anything myself, and I'll force the commit with admin privs.

@ncabatoff ncabatoff merged commit e130fbc into hashicorp:main Nov 10, 2021
@ncabatoff ncabatoff removed this from the 1.9.1 milestone Nov 10, 2021
@ncabatoff
Copy link
Collaborator

Thanks for the contribution! I decided to remove the release label: since this is a change to the client API, there's no reason to backport it, because it won't be compiled in to the Vault binary in any event.

@elsesiy elsesiy deleted the feat/update-raft-autopilot branch November 19, 2021 06:24
qk4l pushed a commit to qk4l/vault that referenced this pull request Feb 4, 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.

API lacks capability to modify Autopilot configuration
3 participants