Tests still fail. I'm also not a fan of removing the defaults from the structs.

TODO

• Preserve defaults while decoding nested structs goccy/go-yaml#260

Despite this is ready I'd wait until either:

• We're going to release RC2
• Preserve defaults while decoding nested structs goccy/go-yaml#260

I wouldn't wait until last minute with merging as having this lying around (hopefully) finished (apart from the replace) when it could also be merged and receive implicit testing by all developers by just running icingadb.

C'mon. There's only one question: Does our config work or not? I consider it already answered by our int. tests.

C'mon. There's only one question: Does our config work or not? I consider it already answered by our int. tests.

We already agreed to just use a patched version get this PR merged.

Well, primarily I don't want to have this lying around in this "yeah well we're still waiting for something, so we don't have to do anything here" state but at least have it reviewed and in a "can be merged at any time" state. And if it's in this state, merging doesn't hurt, removing 2 lines from go.mod would be easy, should it still be merged in time.

I just want to avoid having this as the last thing and then notice that nobody had a closer look in the suggested patch or something like that. Also, the tests are in no way complete. I mean all configs used there are pretty much identical apart from IP addresses, ports and MySQL credentials.

Please rebase.

@lippserd Does your review include the changes done in the github.com/goccy/go-yaml fork?

@lippserd Does your review include the changes done in the github.com/goccy/go-yaml fork?

Yes. The only thing I noticed is that the detection of the initial value is a little different than in the defaults pkg we are using:

reflect.Value.IsValid() vs reflect.DeepEqual(reflect.Zero(field.Type()).Interface(), field.Interface())

But I'm not quite sure what the difference is.

By the way, can't we just set the defaults after the YAML is decoded? 😆

What for?

What for?

We would not have to patch the lib I guess.

We've already patched our Redis lib, so I see no (greater) problem.

We've already patched our Redis lib, so I see no (greater) problem.

If I'm right, we would only have to move 3 lines of code. What is the problem with that?

By the way, can't we just set the defaults after the YAML is decoded? laughing

Do we set defaults for anything where the zero value would be value but not what we want as default?

By the way, can't we just set the defaults after the YAML is decoded? laughing

Do we set defaults for anything where the zero value would be value but not what we want as default?

Both Alex PR and the defaults package rely on zero value detection for applying defaults.

Yes, but there would be a slight change in behavior: right now, if foo has a default and you set foo: 0 in the config, it will continue using 0 as value whereas with your suggested change, it would use the default.

Yes, but there would be a slight change in behavior: right now, if foo has a default and you set foo: 0 in the config, it will continue using 0 as value whereas with your suggested change, it would use the default.

That is true!