-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
@calvn @ncabatoff Could you please take a look? Thanks! |
There was a problem hiding this 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.
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
@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
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 When will this be merged? I see it's tagged for 1.9 but it didn't make it into the rc.. |
@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. |
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. |
The previous implementation only allows retrieving the autopilot state but not modifying it, hence the missing method has been added.
Misc:
Fix #12427