Skip to content

feat: support nested structs, structured arrays, and better secret value handling in config #4727

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

Merged
merged 16 commits into from
Oct 25, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Oct 24, 2022

What this changes:

  • Removes the need for Path field in config, we just use the json tags now
  • Supports slices (does not currently support default values on structured slices)
  • Support nested structs
  • Supports structure array item field value overrides with CODER_$INDEX_$FIELD
  • Safer secrets - by implementing MarshalJSON we can make sure we never return secrets fields over the wire.

@f0ssel f0ssel changed the title feat: support nested structs, structured arrays, and better secret value handling feat: support nested structs, structured arrays, and better secret value handling in config Oct 24, 2022
@f0ssel f0ssel requested a review from a team as a code owner October 24, 2022 19:21
@f0ssel f0ssel requested review from presleyp and kylecarbs and removed request for a team October 24, 2022 19:21
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is structured similarly to the YAML... it's nice!

Comment on lines 130 to 150
return json.Marshal(struct {
Name string `json:"name"`
Usage string `json:"usage"`
Flag string `json:"flag"`
Shorthand string `json:"shorthand"`
Enterprise bool `json:"enterprise"`
Hidden bool `json:"hidden"`
Secret bool `json:"secret"`
Default T `json:"default"`
Value T `json:"value"`
}{
Name: f.Name,
Usage: f.Usage,
Flag: f.Flag,
Shorthand: f.Shorthand,
Enterprise: f.Enterprise,
Hidden: f.Hidden,
Secret: f.Secret,
Default: f.Default,
Value: f.Value,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just marshal ourselves in this case?

json.Marshal(f)

Copy link
Contributor Author

@f0ssel f0ssel Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't! I did that first and you get an infinite loop because it just calls this marshaller again. LOL

@f0ssel
Copy link
Contributor Author

f0ssel commented Oct 24, 2022

I like that this is structured similarly to the YAML... it's nice!

It's exactly the structure, notice there are no mapstructure tags. The json and yaml will always stay in sync this way. I believe in the future we can implement a yaml marshaller and use it to write this struct to yaml, if for some reason we don't want to use viper for that.

@f0ssel f0ssel enabled auto-merge (squash) October 24, 2022 20:36
@f0ssel f0ssel merged commit 585045b into main Oct 25, 2022
@f0ssel f0ssel deleted the f0ssel/recursive-config branch October 25, 2022 00:11
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants