-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
config parse direct hcl #5601
config parse direct hcl #5601
Conversation
This pr depends on the hcl changes in hashicorp/hcl#275 |
WeaklyTypedInput: true, | ||
Result: &config, | ||
// convert strings to time.Durations | ||
err = durations([]td{ |
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.
Could we add additional comments to make it clear that all duration fields will need the string equivalent from now on - probably best to add it in the config struct at the top
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.
@preetapan I added a comment right at the top of agent/config.go
|
||
// this tests for top level keys left by hcl when parsing slices in | ||
// the config structure | ||
func TestConfig_ParseSliceExtra(t *testing.T) { |
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.
nice tests
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.
@preetapan I added an additional sample test to reflect some more real world json configuration files
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 is awesome! So nice seeing so much red.
One question though: one current config parser is case sensitive with respect to fields, but seeing the changes https://github.com/hashicorp/hcl/pull/275/files, I understand that hcl uses strings.EqualFolds
to check field names. Do we want to change config parser to make it case insensitive?
- parse by using hcl.Decode directly - handle time.Duration strings in a second pass - report unexpected keys in a third pass
1e424a6
to
d1dbbe8
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.
LGTM. @notnoop could you also give this a final look over before we merge?
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.
awesome work - thanks!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
address JSON parsing issues by using the hcl parser to directly populate the config struct. The hcl decoder is designed to use the struct as a guide while parsing to resolve syntax ambiguities. With this change, most of the JSON specific behavior is gone (except extra unexpected keys at the top level).
The large diff in config_parse_test is mostly a result of moving the test target data structures to package level vars so that they can be referenced by more than one test.
#1290