-
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 HTTP PATCH support for KV key metadata #13215
Conversation
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.
A few small questions/comments, but looks great overall!
go.mod
Outdated
@@ -108,15 +108,15 @@ require ( | |||
github.com/hashicorp/vault-plugin-secrets-azure v0.11.2 | |||
github.com/hashicorp/vault-plugin-secrets-gcp v0.11.0 | |||
github.com/hashicorp/vault-plugin-secrets-gcpkms v0.10.0 | |||
github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211123171606-16933c88368a | |||
github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211206210934-74fe79f60b74 |
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.
Were these upgrades intentional? If you wanted to include the upgrades in the other 2 PRs open for review, I think you'd need to re-import after those PRs are merged.
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.
Yes, the intention is to include the upgrade for vault-plugin-secrets-kv
for review. As you mentioned, I plan to re-import after those PRs are merged.
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.
Looks good to me, though as a minor nit I'd still prefer for us to clean up the BoolVar
struct if we can, since it doesn't seem to be used. Overall it looks good to go!
* go get vault-plugin-secrets-kv@vault-4290-patch-metadata * add kv metadata patch command * add changelog entry * success tests for kv metadata patch flags * add more kv metadata patch flags tests * add kv metadata patch cas warning test * add kv-v2 key metadata patch API docs * add kv metadata patch to docs * prevent unintentional field overwriting in kv metadata put cmd * like create/update ops, prevent patch to paths ending in / * fix kv metadata patch cmd in docs * fix flag defaults for kv metadata put * go get vault-plugin-secrets-kv@vault-4290-patch-metadata * fix TestKvMetadataPatchCommand_Flags test * doc fixes * go get vault-plugin-secrets-kv@master; go mod tidy
PR #57 for
vault-plugin-secrets-kv
introduces aPatchOperation
handler for the KV<mount>/metadata/:path
endpoint. This PR introduces the CLI commandkv metadata patch
. The command will only include fields in the request data that have been provided as flags to prevent unexpectedly clearing them via the patch operation. This was actually an issue that was discovered for thekv metadata put
command and has been fixed as part of this PR.