rationalizing Config constructors / building

[shawkins]

Description

@manusa @rohanKanojia separated this from the pr dealing with watchLists. I realize that using just the swicth to use a static constructor is pretty narrow change. If possible I'd like to shoot for something bigger. The changes here get rid of all of our messiest constructors and hopely make adding new options more straight-forward.

• SundrioConfig -> goes from extending the Config to being a base class (could be renamed BaseConfig or something).

  • There are two reasons to do this:
    • Creates a clean pojo from which to do sundrio generation.
    • Creates a clean pojo from which to do delegated Json deserialization - see the Config(SundrioConfig config) constructor.
  • Downsides:
    • Probably needs to be public (Private configuration classes cause trouble with Java native (reflection) #4724) - or maybe just the class and not the constructor. Since all the apis expect Config, not SundrioConfig, there's not much that can be done with it. If we didn't have the need for overriding ConfigFluent methods, we could probably do away with SundrioConfig (see below for similar reasoning with OpenshiftConfigFluent)
  • Latent issues:
    • additionalProperties and defaultNamespace do not seem to be handled properly (missing) in the current SundrioConfigFluent.

• ConfigFluent can just reuse the base logic, no need to mess with updating fluents when a new option is added

• You do still have to update the Config copy constructor to copy the value from the SundrioConfig.

• For Openshift the paradigm flips. There's now an SundrioOpenShiftConfig extending OpenshiftConfig with the dummy fields moved there.

  • I believe we could get rid of this class and the hardcoded OpenshiftConfigFluent/Builder if the logic in the copy constructor (if x is not null or empty, set x) were moved to the setter methods as that can all be done in the default sundrio logic post construction.

Other latent issues:

• We're subtlely changing the meaning of several of the ConfigBuilder consturctor methods, not sure if we should address that now that the change is already in.
• autoConfigure behavior seems inconsistent. If you do new ConfigBuilder().build() we'll use autoConfigure unless disabled, but if you parse we will only use autoConfigure if the field is true.
• OpenShiftConfig wrapping can rerun autoConfigure.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[shawkins]

To reply to #7190 (comment)

There are three options:

• Have SundrioOpenShiftConfig extend SundrioConfig - this does not work well because then OpenshiftConfig can't easily extend Config.
• Have a SundrioOpenShiftConfig extend OpenShiftConfig (this pr). We still have to take responsibility for creating an OpenShift Fluent and Builder, and any new OpenShift options will be a pain / spread out to add.
• Just have OpenShiftConfig - the main problem here is that the generated builder will not be compatible with our logic. Moving the post default / autoconfigure constructor logic into setters could be breaking for anyone removing defaults via set methods - if we don't think this could or should happen, then we can could take this path instead.

@manusa @rohanKanojia this should be ready, but it looks like there is some flakeyness to one of the jdk tests. See the main pr comment for more of the thinking behind these changes.

[manusa]

but it looks like there is some flakyness to one of the jdk tests

I'm not sure of the reason why, but tests have become extremely brittle and flaky.
This was not the case a couple of months ago and there have not been significant code changes that justify it.
There must either be issues with the dependencies that are less reliable now, or the GitHub actions environment.

Regarding the changes in general. I'm still unsure why this is necessary but if it helps then I'm happy with the changes.

... logic into setters

The main goal for the config simplification was to make it as immutable as possible and to only allow its interaction via builders. Both are impossible given the original situation and the uses that people have been giving to the class. In any case, I'd still strive to remove the setters and any logic bound into them as much as possible.

[shawkins]

I'm still unsure why this is necessary but if it helps then I'm happy with the changes.

Sorry, I had hoped the value would have been clearer. Primarily it removes the very large constructors and any confusion that may arrise from updating them. Next it removes some of the steps of adding a new config property. There should just be two steps to add something like watchLists for the other pr - add a field on SundrioConfig, then copy the value from the SundrioConfig to the Config in the Config constructor. Finally it highlights issues with a couple of existing options, such as defaultNamespace and additionalProperties.

In any case, I'd still strive to remove the setters and any logic bound into them as much as possible.

Config.setMasterUrl is probably the biggest exception to having logic in the setter.

For the rest it's mostly a semantic issue with default handling. For example Config config = new ConfigBuilder().build(); config.setApiVersion(null); - is expected to force a null api version, while new ConfigBuilder().withApiVersion(null).build() - is expected to use the default api version.

[manusa]

I've been digging a little and I think the if (Boolean.TRUE.equals(shouldSetDefaultValues)) { was added for deserialization purposes only.
At the time 08b0e9f / #6153, there wasn't a constructor annotated with @JsonCreator. I'm still not quite sure why it was added in the first place.
Anyway, I think we can safely remove that fork and always set the defaults (in the constructor).

For the rest it's mostly a semantic issue with default handling. For example Config config = new ConfigBuilder().build(); config.setApiVersion(null); - is expected to force a null api version, while new ConfigBuilder().withApiVersion(null).build() - is expected to use the default api version.

This is precisely the issue with the setters.
Config should ideally be managed only through the ConfigBuilder.

additionalProperties and defaultNamespace do not seem to be handled properly (missing) in the current SundrioConfigFluent.

I can't see a way how defaultNamespace is actually consumed.
Similar to additionalProperties, I don't think these are applicable for this class.

autoConfigure behavior seems inconsistent. If you do new ConfigBuilder().build() we'll use autoConfigure unless disabled, but if you parse we will only use autoConfigure if the field is true.

autoConfigure is opt-out, can only be skipped by using Config.empty(), by using new ConfigBuilder(Config.empty() or by new ConfigBuilder().withAutoConfigure(false)

Config parsing is the edge case which ideally should be unsupported (however, I'm unsure about who might be actually doing this).

OpenShiftConfig wrapping can rerun autoConfigure.

This can be an issue if the user has opted out and is expecting to have a clean OpenShiftConfig.

[shawkins]

Anyway, I think we can safely remove that fork and always set the defaults (in the constructor).

Do you want me to do that in this pr?

I can't see a way how defaultNamespace is actually consumed.

It's used via config.isDefaultNamespace() in OperationContext.

Similar to additionalProperties, I don't think these are applicable for this class.

They are set upon deserialization. If you have anything unrecognized, or are even just parsing an OpenShiftConfig as a Config, then it will be populated. If you currently do something like parsedConfig.edit().build() - the additionalProperties are lost.

Config parsing is the edge case which ideally should be unsupported (however, I'm unsure about who might be actually doing this).

I'm a little fuzzy on that too.

This can be an issue if the user has opted out and is expecting to have a clean OpenShiftConfig.

This is happening specifically with the public OpenShiftConfig(Config kubernetesConfig) style constructors. If you have opted out, then auto configure won't be run either time, but in the other scenarios it will either run twice or as highlighted with the parsing case won't run when parsed, but then will run when wrapped.

[manusa]

It's used via config.isDefaultNamespace() in OperationContext.

Yes, I did find that, but it's not set anywhere. It also doesn't make sense to set it in the ConfigBuilder. That's why I'm unsure this is actually ever used.

They are set upon deserialization. If you have anything unrecognized, or are even just parsing an OpenShiftConfig as a Config, then it will be populated. If you currently do something like parsedConfig.edit().build() - the additionalProperties are lost.

Yes, but is this a real use-case? i.e. something that can actually happen.

This is happening specifically with the public OpenShiftConfig(Config kubernetesConfig) style constructors. If you have opted out, then auto configure won't be run either time, but in the other scenarios it will either run twice or as highlighted with the parsing case won't run when parsed, but then will run when wrapped.

The parsing scenarios are the ones that worry me the least. They shouldn't really be supported. I sort of recall that this could be used in IDE tooling, but hopefully they might have refactored that part.

[shawkins]

Yes, I did find that, but it's not set anywhere. It also doesn't make sense to set it in the ConfigBuilder. That's why I'm unsure this is actually ever used.

It was supposed to be configurable.

Yes, but is this a real use-case? i.e. something that can actually happen.

I'd want to keep things as regular and predicable as possible.