fix: loading of dbUsername field for server config #2189
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
requested review from
BruceMacD,
jmorganca,
pdevine and
kimskimchi
as code owners
dnephin
force-pushed
the
dnephin/fix-server-config
branch
from
38fe290 to
6aa9317
Compare
dnephin
commented
Jun 3, 2022
| @@ -50,32 +51,6 @@ func TestServerCmd_LoadOptions(t *testing.T) { | |||
| } | |||
|
|
|||
| testCases := []testCase{ | |||
| { | |||
| name: "secret providers", | |||
There was a problem hiding this comment.
This test case is now part of "all options from config" below.
Comment on lines
-41
to
-46
| Server string `mapstructure:"server"` | ||
| Name string `mapstructure:"name"` | ||
| AccessKey string `mapstructure:"accessKey"` | ||
| CACert string `mapstructure:"caCert"` | ||
| CAKey string `mapstructure:"caKey"` | ||
| SkipTLSVerify bool `mapstructure:"skipTLSVerify"` |
There was a problem hiding this comment.
This change is covered by an existing test: cmd.TestConnectorCmd
| } | ||
|
|
||
| type KeyProvider struct { | ||
| Kind string `mapstructure:"kind" validate:"required"` | ||
| Kind string `validate:"required"` |
There was a problem hiding this comment.
The key provider and secret provider loading is covered by:
server.TestKeyProvider_PrepareForDecode_IntegrationWithDecode_FullConfigserver.TestSecretProvider_PrepareForDecode_IntegrationWithDecode_FullConfig
| @@ -8,7 +8,7 @@ import ( | |||
| ) | |||
|
|
|||
| type OptionsDiffV0dot1 struct { | |||
| Identities []User `mapstructure:"identities" validate:"dive"` | |||
| Identities []User `validate:"dive"` | |||
There was a problem hiding this comment.
Covered by cmd.TestApplyVersion0ConfigToLatest
| DBHost string | ||
| DBPort int | ||
| DBName string | ||
| DBUsername string |
There was a problem hiding this comment.
The primary fix is this one line
ssoroka
approved these changes
Jun 3, 2022
BruceMacD
approved these changes
Jun 6, 2022
mxyng
approved these changes
Jun 6, 2022
Add a test that covers all config fields. Also remove mapstructure tags, which were not being used since we changed to using cliopts for config loading. Co-authored-by: Bruce MacDonald <[email protected]>
dnephin
force-pushed
the
dnephin/fix-server-config
branch
from
9396066 to
b81a4ab
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #1755 we changed from using
mapstructuretags to usingconfigtags, but I forgot to remove or update all the existing struct tags.Every other field continued to work because the struct field name matched the convention. Unfortunately
dbUsernamewas the one case where it did not match convention and required the struct tag. A workaround was to usedbUserorINFRA_SERVER_DB_USER.This PR fixes the bug by renaming the field to
DBUsername.It also adds test coverage for every field in
server.Optionsto show this is the only field with the problem.I also removed the struct tags, since they no longer do anything and keeping them would be confusing. I'll leave some inline comments for where each of the fields is tested.