Make fly config validate detect unknown fields #3544
Conversation
While we have a number of validators that will check to make sure that field _values_ are correct, we surprisingly do not check to make sure theat field _names_ are correct. Instead we silently ignore fields that have unknown names, often leading to difficult to debug situations. This change makes use of `decoder.DisallowUnknownFields` to enable reporting on these errors by fly config validate.
|
Amusingly, one of the CI tests is now failing due to an invalid
What is the current thinking on validation checks? Adding a validation check can break future deploys. Should it (in general)? Is this specific case different? We can make unknown field validation messages a warning only by capturing and resetting |
that is my main concern, every time we change something to be more strict, it breaks users' automated deployments in some way. I think we have to start by showing big warnings first. |
flyctl command that parses a config file.
|
|
||
| decoder := json.NewDecoder(bytes.NewReader(newbuf)) | ||
| decoder.DisallowUnknownFields() | ||
| err = decoder.Decode(cfg) |
There was a problem hiding this comment.
can we add a test case and its test file at internal/appconfig/testdata/ ?
I wonder if cfg is "filled" with the rest of the values even when there is an error due to unknown fields.
While we have a number of validators that will check to make sure that field values are correct, we surprisingly do not check to make sure theat field names are correct. Instead we silently ignore fields that have unknown names, often leading to difficult to debug situations.
This change makes use of
decoder.DisallowUnknownFieldsto enable reporting on these errors by fly config validate.