Only warn about unknown config options instead of rejecting them #631

julianbrost · 2023-08-03T15:35:56Z

This is a quick proof of concept for an idea I had how to make #605 more suitable for a bug fix release.

Currently, it would just fail with an error. This PR basically parses the config like v1.1.0 did, but additionally parses the config a second time with yaml.DisallowUnknownField() (like the current master and therefore v1.2 will do) and issue a warning if it returns an error.

The icingadb-migrate command still treats unknown options as an error. This is more of a manual one-shot tool, so making it an error there right away shouldn't break anything.

Tests

Config with unknown options and other errors

First prints error and then additionally the warning:

$ go run ./cmd/icingadb -c <(echo unknown: 42)       
invalid configuration: database host missing

warning: ignored unknown config option:

[1:1] unknown field "unknown"
    >  1 | unknown: 42
           ^

Unknown option in an otherwise valid config

Logs the warning and then continues to run:

$ go run ./cmd/icingadb -c config.yml
2023-08-04T12:06:33.213+0200	WARN	icingadb	ignoring unknown config option, this will become a fatal error in Icinga DB v1.2:

[41:1] unknown field "unknown"
      38 |   options:
      39 |     notification: 14
      40 | 
    > 41 | unknown: 42
           ^
2023-08-04T12:06:33.213+0200	INFO	icingadb	Starting Icinga DB
[...running normally...]

Al2Klimov

Not bad. My only complaint is the wrong base branch. (This PR won't be included in v1.2, will it?)

julianbrost · 2023-08-04T10:10:30Z

I've pushed a new version that delays the warning until logging is initialized. In case there is an error (and logging won't be initialized), the warning is embedded in the error message.

Al2Klimov

Even better (for the ones who read logs 🪵 at all).

pkg/config/config.go

Al2Klimov · 2023-08-04T10:56:22Z

pkg/config/config.go

 	if err := c.Validate(); err != nil {
-		return nil, errors.Wrap(err, "invalid configuration")
+		msg := "invalid configuration"
+		if warn := c.DecodeWarning; warn != nil {


What's warn for?

Shorthand for the longer name.

A bit too much of shorthand. But this is a matter of opinion. Same for "invalid configuration".

I was just trying hard to avoid breaking the next line, especially somewhere in the middle of the format string.

cmd/icingadb/main.go

Al2Klimov · 2023-08-04T10:57:45Z

pkg/config/config.go

 	if err := c.Validate(); err != nil {
-		return nil, errors.Wrap(err, "invalid configuration")
+		msg := "invalid configuration"


This could be a const or even inlined.

pkg/config/config.go

Al2Klimov · 2023-08-04T11:04:15Z

pkg/config/config_test.go

 		},
 		{
 			name:   "mini-with-unknown",
 			input:  miniConf + "\nunknown: 42",
-			output: nil,


This was the only one of its kind. You've made the existing if below obsolete.

Al2Klimov

Do we want a similar change in icingadb-migrate? This is more of a manual one-shot tool, so making it an error there right away shouldn't break anything.

You are right. To be even more specific, there are three kinds of users:

have done migration – won't use the tool again – not affected
doing migration – affected on interrupt – solutions:
- wait with the update
- just adjust the config
didn't start migration – didn't use the tool yet – not affected by my change as such (or yours)

Al2Klimov · 2023-08-04T14:44:18Z

pkg/config/config.go

 	if err := c.Validate(); err != nil {
-		return nil, errors.Wrap(err, "invalid configuration")
+		msg := "invalid configuration"
+		if warn := c.DecodeWarning; warn != nil {


A bit too much of shorthand. But this is a matter of opinion. Same for "invalid configuration".

…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 this to the 1.1.1 milestone Aug 3, 2023

cla-bot bot added the cla/signed label Aug 3, 2023

Al2Klimov reviewed Aug 3, 2023

View reviewed changes

julianbrost force-pushed the unknown-config-option-warning branch from 9215bf2 to 4566091 Compare August 4, 2023 10:06

Al2Klimov reviewed Aug 4, 2023

View reviewed changes

julianbrost force-pushed the unknown-config-option-warning branch from 4566091 to 0f0c03d Compare August 4, 2023 14:37

julianbrost requested a review from Al2Klimov August 4, 2023 14:38

Al2Klimov approved these changes Aug 4, 2023

View reviewed changes

julianbrost mentioned this pull request Aug 7, 2023

Release 1.1.1 #629

Merged

julianbrost changed the base branch from main to support/1.1 August 8, 2023 09:27

cmd/icingadb: warn about unknown config options instead of rejecting …

d11f03d

…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 force-pushed the unknown-config-option-warning branch from 0f0c03d to d11f03d Compare August 8, 2023 09:28

julianbrost marked this pull request as ready for review August 8, 2023 09:30

julianbrost requested a review from Al2Klimov August 8, 2023 09:30

Al2Klimov approved these changes Aug 8, 2023

View reviewed changes

julianbrost merged commit 56e5a7b into support/1.1 Aug 8, 2023
30 of 31 checks passed

julianbrost deleted the unknown-config-option-warning branch August 8, 2023 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only warn about unknown config options instead of rejecting them #631

Only warn about unknown config options instead of rejecting them #631

julianbrost commented Aug 3, 2023 •

edited

Loading

Al2Klimov left a comment

julianbrost commented Aug 4, 2023

Al2Klimov left a comment

Al2Klimov Aug 4, 2023

julianbrost Aug 4, 2023

Al2Klimov Aug 4, 2023

julianbrost Aug 4, 2023

Al2Klimov Aug 4, 2023

Al2Klimov Aug 4, 2023

Al2Klimov left a comment

Al2Klimov Aug 4, 2023

Only warn about unknown config options instead of rejecting them #631

Only warn about unknown config options instead of rejecting them #631

Conversation

julianbrost commented Aug 3, 2023 • edited Loading

Tests

Config with unknown options and other errors

Unknown option in an otherwise valid config

Al2Klimov left a comment

Choose a reason for hiding this comment

julianbrost commented Aug 4, 2023

Al2Klimov left a comment

Choose a reason for hiding this comment

Al2Klimov Aug 4, 2023

Choose a reason for hiding this comment

julianbrost Aug 4, 2023

Choose a reason for hiding this comment

Al2Klimov Aug 4, 2023

Choose a reason for hiding this comment

julianbrost Aug 4, 2023

Choose a reason for hiding this comment

Al2Klimov Aug 4, 2023

Choose a reason for hiding this comment

Al2Klimov Aug 4, 2023

Choose a reason for hiding this comment

Al2Klimov left a comment

Choose a reason for hiding this comment

Al2Klimov Aug 4, 2023

Choose a reason for hiding this comment

julianbrost commented Aug 3, 2023 •

edited

Loading