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>
georgethebeatle
force-pushed
the
autocomplete-kv-v2
branch
from
568c972
to
a916d75
Compare
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.
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?
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.
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(...)
}
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
|
@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 putandkv patchcommands