Expose unknown fields and duplicate sections as diagnose warnings #11455
Conversation
| for _, ex := range expected { | ||
| // Only test the string, pos may change | ||
| if ex.Problem == er1.Problem { | ||
| continue outer |
There was a problem hiding this comment.
could these be refactored to just break and not have the tags?
Also, what are those tags called? I've only seen them with GOTO
There was a problem hiding this comment.
And I ask about refactoring (at least for this loop) because i think it might be easier for another eng to understand down the road (i think it would have taken me less time to get it)
There was a problem hiding this comment.
They're block labels. The idea there was if you just break on the inner one you'll hit the t.Fatal, which isn't what we want. That said I can do it with a found bool instead.
There was a problem hiding this comment.
cool, i haven't seen that used before! closest i've seen is with goto. i'm gonna read about them!
do whatever you think is most readable, i'm not too concerned about this
There was a problem hiding this comment.
internalshared/configutil/config.go
Outdated
| @@ -13,6 +14,10 @@ import ( | |||
|
|
|||
| // SharedConfig contains some shared values | |||
| type SharedConfig struct { | |||
| FoundKeys []string `hcl:",decodedFields"` | |||
| UnusedKeys map[string][]token.Pos `hcl:",unusedKeyPositions"` | |||
There was a problem hiding this comment.
did you want this to be of type UnusedKeyMap as defined in config_util.go?
There was a problem hiding this comment.
Ah nice, I must have added that type later but didn't loop back.
| func (ec *EntSharedConfig) ParseConfig(list *ast.ObjectList) error { | ||
| return nil | ||
| } | ||
|
|
||
| func ParseEntropy(result *SharedConfig, list *ast.ObjectList, blockName string) error { | ||
| return nil | ||
| } | ||
|
|
||
| // Creates the ConfigErrors for unused fields, which occur in various structs | ||
| func ValidateUnusedFields(unusedKeyPositions map[string][]token.Pos, source string) []ConfigError { |
There was a problem hiding this comment.
did you want the type to be UnusedKeyMap here too?
There was a problem hiding this comment.
and in a few other places in this file
|
|
||
| func UnusedFieldDifference(a, b map[string][]token.Pos, foundKeys []string) map[string][]token.Pos { | ||
| if a == nil { | ||
| return nil |
There was a problem hiding this comment.
we have different behavior with a being nil vs b. could we document what a and b are supposed to be so that future users of this code know that order matters?
| StatsiteAddr string `hcl:"statsite_address"` | ||
| StatsdAddr string `hcl:"statsd_address"` | ||
| FoundKeys []string `hcl:",decodedFields"` | ||
| UnusedKeys map[string][]token.Pos `hcl:",unusedKeyPositions"` |
| @@ -32,6 +32,16 @@ func StrListContains(haystack []string, needle string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // StrListContainsCaseInsensitive looks for a string in a list of strings. | |||
| func StrListContainsCaseInsensitive(haystack []string, needle string) bool { | |||
There was a problem hiding this comment.
love the variable names :)
There was a problem hiding this comment.
Yeah, that's pretty common in other APIs. Suprised Go didn't use it for things like strings.Contains
command/operator_diagnose.go
Outdated
| @@ -155,7 +155,11 @@ func (c *OperatorDiagnoseCommand) RunWithParsedFlags() int { | |||
| c.UI.Output(err.Error()) | |||
| return 1 | |||
| } | |||
|
|
|||
| errors := config.Validate("test") | |||
There was a problem hiding this comment.
Nit: can we change source for Validate to something like filename to make this more clear? Or sourceFilePath, or something of that nature which denotes it is a file path?
| } | ||
| res := make(map[string][]token.Pos) | ||
| for k, v := range a { | ||
| if _, ok := b[k]; !ok && !strutil.StrListContainsCaseInsensitive(foundKeys, govalidator.UnderscoreToCamelCase(k)) { |
There was a problem hiding this comment.
Why does k need to be converted to govalidator.UnderscoreToCamelCase ?
There was a problem hiding this comment.
Because HCL's AST refers to the struct member name, while the found keys uses the string from the .hcl file (in lower case with underscores)
| expected := []configutil.ConfigError{ | ||
| { | ||
| Problem: "unknown field bad_value found in configuration", | ||
| Position: token.Pos{}, |
There was a problem hiding this comment.
I don't think the Pos is being checked in the test. If possible, could we see this in action?
| @@ -57,3 +57,7 @@ func TestConfigRaftRetryJoin(t *testing.T) { | |||
| func TestParseSeals(t *testing.T) { | |||
| testParseSeals(t) | |||
| } | |||
|
|
|||
| func TestUnknownFieldValidation(t *testing.T) { | |||
There was a problem hiding this comment.
This is great! Can we also have a test that checks the filename for unusedKeys is correct, for configs between multiple files?
HridoyRoy
dismissed
their stale review
I didn't look closely enough at the test failures, my apologies!
…shicorp#11455) * Expose unknown fields and duplicate sections as diagnose warnings * section counts not needed, already handled * Address PR feedback * Prune more of the new fields before tests call deep.Equals * Update go.mod
…shicorp#11455) * Expose unknown fields and duplicate sections as diagnose warnings * section counts not needed, already handled * Address PR feedback * Prune more of the new fields before tests call deep.Equals * Update go.mod
No description provided.