Skip to content
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 parsing: fail on unknown fields and print them #605

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Jun 19, 2023

Useful against config validation or runtime failures caused by wrong field spelling or YAML indentation.

➜  icingadb git:(yaml.DisallowUnknownField) ✗ go run ./cmd/icingadb-migrate -c i2i.yml -t `mktemp -d`
can't parse config file: [1:1] unknown field "Icinga2"
>  1 | Icinga2:
       ^
   2 |   env: da39a3ee5e6b4b0d3255bfef95601890afd80709
   3 | ido:
   4 |   type: mysql
   5 |
exit status 2

refs #574

TODO

  • review (non-author)
  • unit test that particular case in FromYAMLFile() (author)

Useful against config validation or runtime failures
caused by wrong field spelling or YAML indentation.
@Al2Klimov Al2Klimov added the bug Something isn't working label Jun 19, 2023
@cla-bot cla-bot bot added the cla/signed label Jun 19, 2023
@julianbrost
Copy link
Contributor

Looks fine in general, just a few things:

  1. I wouldn't really consider this a bug (fix), just a more helpful error message.
  2. Shouldn't this fix the linked issue?
  3. There are some places that are just a map[string]... to the YAML parser, so there you can still miss typos. For example:
    logging:
      options:
        something-that-might-have-a-typo: debug
    Nothing that should stop this PR, but it might be worth a look if we can add something to the Validate() functions that checks that there are no unexpected keys there.

@Al2Klimov
Copy link
Member Author

Regarding the issue, this is only half of the rent. But this is already an improvement on its own.

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

review (non-author)

The code changes look fine to me.

@julianbrost julianbrost removed the bug Something isn't working label Jul 7, 2023
@Al2Klimov Al2Klimov self-assigned this Jul 7, 2023
@Al2Klimov Al2Klimov removed their assignment Jul 11, 2023
@julianbrost julianbrost added this to the 1.2.0 milestone Jul 25, 2023
@julianbrost julianbrost merged commit 536c808 into master Jul 25, 2023
17 of 18 checks passed
@julianbrost julianbrost deleted the yaml.DisallowUnknownField branch July 25, 2023 08:29
julianbrost added a commit that referenced this pull request Aug 4, 2023
…them

This makes the change from #605 more suitable for the v1.1.1 bug fix release
because this way, no new fatal errors are added while keeping the helpful
information in a warning.
julianbrost added a commit that referenced this pull request Aug 4, 2023
…them

This makes the change from #605 more suitable for the v1.1.1 bug fix release
because this way, no new fatal errors are added while keeping the helpful
information in a warning.
julianbrost added a commit that referenced this pull request Aug 8, 2023
…them

This makes the change from #605 more suitable for the v1.1.1 bug fix release
because this way, no new fatal errors are added while keeping the helpful
information in a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants