Revert default config

[eleftherias]

Summary

Revert the change which takes into account the {} default value in a config field.
We were running into an issue where the SQL port was defaulted to 0, rather than the expected 5432 value.
https://github.com/stacklok/minder/blob/6186d553c6bb6eb83aedd9679bdb2b744ad7ddb8/internal/config/common.go#L38

Change Type

Mark the type of change your PR introduces:

• Bug fix (resolves an issue without affecting existing features)
• Feature (adds new functionality without breaking changes)
• Breaking change (may impact existing functionalities or require documentation updates)
• Documentation (updates or additions to documentation)
• Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

• Reviewed my own code for quality and clarity.
• Added comments to complex or tricky code sections.
• Updated any affected documentation.
• Included tests that validate the fix or feature.
• Checked that related changes are merged.

[coveralls]

coverage: 47.698% (-0.004%) from 47.702%
when pulling c70bf93 on eleftherias:revert-default-config
into f31c850 on stacklok:main.

[Vyom-Yadav]

I get that this was done to maybe fix production immediately. But isn't the logically correct thing to remove default:"{}" from wherever it was causing an issue? https://github.com/stacklok/minder/blob/66179b892f89e053aa0766916b03f5088e686b5f/internal/config/server/events.go#L47-L51

What's the point of having default:"{}" if it's not honoured? That's just leaving a bug in.

[evankanderson]

The symptom (which we apparently don't have test coverage for) was that the DatabaseConfig.port for Minder was being zeroed in staging and production, which caused Minder to fail to start.

I think we need to figure out a different way to configure this, and add a test that will prevent the misconfiguration from causing crashing servers.

We also have an action item to investigate why our alerting didn't immediately complain when we had crashing servers, so we only noticed it several hours later.

[evankanderson]

(But, as it is, the values in EventConfig are dangerous in combination with the code to not initialize items with default:"{}" tags.)

[evankanderson]

I think the right answer is to remove those (ignored) defaults, and then add a complaint if they sneak back in...

[evankanderson]

Going through this code again, I think the exciting thing that got missed in #2633 is that if you skip calling setViperStructDefaults on the sub-message, fields don't merge properly between configs if you have multiple layers, and can't be set using environment variables. (See the comment at the top of setViperStructDefaults.)

Since this just bit us, I'll be working on a set of patches to roll this safely forward again. Did you need the ability to clear out the defaults from the structs that you added in #2633, or were you just trying to be complete?

[Vyom-Yadav]

I think the exciting thing that got missed in #2633 is that if you skip calling setViperStructDefaults on the sub-message, fields don't merge properly between configs if you have multiple layers, and can't be set using environment variables

Right, with that, we dropped the ability to set things using env variables for struct fields with default:"{}".

For testing, if possible, I'd suggest having a test that replicates non-confidential server configuration options to ensure that the input configuration gives the expected complete (all options) output. That way, we can bridge some gaps between SAAS and OSS.

Did you need the ability to clear out the defaults from the structs that you added in #2633, or were you just trying to be complete?

I sort of needed that. The problem is that types like db connection are shared by multiple fields, and having the same defaults doesn't make sense. So zeroing them using default:"{}" looked like a better option. The user explicitly specifies that using config. Also, now that you mentioned env vars above, I think validation in #2638 doesn't check env vars. It gives an error based on just the config file. I'll fix that portion.

[JAORMX]

The symptom (which we apparently don't have test coverage for) was that the DatabaseConfig.port for Minder was being zeroed in staging and production, which caused Minder to fail to start.

I think we need to figure out a different way to configure this, and add a test that will prevent the misconfiguration from causing crashing servers.

We also have an action item to investigate why our alerting didn't immediately complain when we had crashing servers, so we only noticed it several hours later.

Another issue was the EEA, which ended configured with a zero value in the aggregator interval. Which ended up effectively disabling the lock. We really need better testing in general.

[evankanderson]

See #2836 for a fix to flag the default:"{}" and remove them, and then #2839 to enable this case:

For testing, if possible, I'd suggest having a test that replicates non-confidential server configuration options to ensure that the input configuration gives the expected complete (all options) output. That way, we can bridge some gaps between SAAS and OSS.

I think the test added in #2836 effectively does this, for EEA. I'm not sure that testing each option's pass-through in all configurations is going to be a good use of effort -- as an example, Coveralls currently reports 60% coverage for internal/config. At the same time, we do need a mechanism to check that a prototypical configuration produces a reasonable outcome.

Maybe it makes sense to extend the --dump_config option to dump something more human-readable and then have a test that runs go run ./cmd/server serve --dump_config $OTHER_FLAGS and compares the output to what we expect?

Did you need the ability to clear out the defaults from the structs that you added in #2633, or were you just trying to be complete?

I sort of needed that. The problem is that types like db connection are shared by multiple fields, and having the same defaults doesn't make sense. So zeroing them using default:"{}" looked like a better option. The user explicitly specifies that using config. Also, now that you mentioned env vars above, I think validation in #2638 doesn't check env vars. It gives an error based on just the config file. I'll fix that portion.

I think the two PRs above (particularly #2839) should cover your use case.