-
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
Make key completion work for both kv-v1 and kv-v2 #16553
Conversation
Co-authored-by: Kieron Browne <kbrowne@vmware.com> Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com> Co-authored-by: Danail Branekov <danailster@gmail.com>
568c972
to
a916d75
Compare
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.
Hi, thanks for submitting this PR. Left a few test related comments, but the functional code changes look good to me.
vault/testing.go
Outdated
@@ -2041,7 +2042,7 @@ func (tc *TestCluster) initCores(t testing.T, opts *TestClusterOptions, addAudit | |||
"path": "secret/", | |||
"description": "key/value secret storage", | |||
"options": map[string]string{ | |||
"version": "1", | |||
"version": opts.KVVersion, |
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.
It looks like opts.KVVersion
in initCores is causing a panic in some tests that use initCores
because there's no default. Could you please add a default value?
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.
done
command/command_test.go
Outdated
} | ||
|
||
// testVaultServerCoreConfig creates a new vault cluster with the given core | ||
// configuration. This is a lower-level test helper. | ||
func testVaultServerCoreConfig(tb testing.TB, coreConfig *vault.CoreConfig) (*api.Client, []string, func()) { | ||
func testVaultServerCoreConfig(tb testing.TB, coreConfig *vault.CoreConfig, opts *vault.TestClusterOptions) (*api.Client, []string, func()) { |
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.
This function is also used in external-tests
. I think it would be simpler to leave this function unchanged so we don't have to worry about fixing tests that aren't relevant to this change, and instead adding a new function. For example,
testVaultServerCoreConfigWithOpts(...opts) {
...copy code from testVaultServerCoreConfig here and initialize the cluster with opts
}
testVaultServerCoreConfig(...) {
return testVaultServerCoreConfigWithOpts(...)
}
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.
done
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
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.
Lgtm!
@danail-branekov would it be possible for you to sign the CLA please? (https://github.com/hashicorp/vault/blob/main/CONTRIBUTING.md#contributor-license-agreement) |
CLA signed, thanks! |
@HridoyRoy @peteski22 it looks like the test failures for this are not related to the PR - can you help us get them to green? Thanks! |
The test failures were indeed unrelated, seems like something changed on CircleCI's side. There's a fix in main, but I didn't want to require you to apply it, since your changes aren't likely to break the remote docker tests while leaving the regular tests passing. Thanks for the contribution! |
Fixes #16552
kv get
,kv put
andkv patch
commands