enhance: add support for streaming lists

[shawkins]

Adds support for informers to use watch / streaming lists. This is currently enabled via Reflector.setStreamingList

TODO:

• how to allow wiring of streamingList=true
  • to do this automatically we'd need to check both the server version and feature gates
  • could be a setting on the Config (like the existing watch settings)
  • could also be exposed per informer

I'm leaning toward doing the latter two and not doing this automatically for now.

• additional validation

  • If you set a Limit or other continuation related fields it will simply be ignored - I don't think we have to take responsiblity for this

• other api methods, see Add support for informer watching without pagination #5081 (comment)

  • added a Watchable.streamingList method

closes: #5081

Description

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]

Added the Config wiring to enable using this feature. Checking the server for feature gate enablement seems a bit hackish - a raw request against /metrics and then scraping for the feature and a 1 value - so I'd like to just leave this as the only mechanism initially.

I would also leave off any built-in mechanism to use a watch list under the covers for a regular list operation.

@manusa @rohanKanojia having two similar constructors on Config seems dangerous (it caused me a little trouble here). Are you good with having a static method for the JsonCreator? We can even javadoc that as something that should not be called directly.

@csviri is there anything you would like to see done here?

[csviri]

Hi @shawkins , look good to me, thank you!

Added the Config wiring to enable using this feature. Checking the server for feature gate enablement seems a bit hackish - a raw request against /metrics and then scraping for the feature and a 1 value - so I'd like to just leave this as the only mechanism initially.

I guess you mean this:
kubernetes/kubernetes#87869 (comment)

It is true that it is hacky, maybe we can experiment with it as the next iteration. Later if this would be enabled on K8S by default could be enabled based on version I guess.

Wonder if this listing should be extended by simple list eventually:

context.getClient().resources(TestCustomResource.class).list();

That could be also beneficial.

[shawkins]

It is true that it is hacky, maybe we can experiment with it as the next iteration. Later if this would be enabled on K8S by default could be enabled based on version I guess.

Using the version would be much preferred. I guess in a rare case you could still disable the feature gate in a late release, so users could still be on the hook for manually toggling the Config value.

Wonder if this listing should be extended by simple list eventually

To match the current behavior we need to return a typed KubernetesResourceList<T>.

• We lack a good way of constructing one that is modifable (PodList etc. don't have a common base impl), so we'd need to add setter methods onto KubernetesResourceList
• We won't easily know what the list kind is when dealing with a generic resource, we'd probably just have to resort to "List" - but that is a behavioral change.

[manusa]

@manusa @rohanKanojia having two similar constructors on Config seems dangerous (it caused me a little trouble here). Are you good with having a static method for the JsonCreator? We can even javadoc that as something that should not be called directly

The multiple config constructors are there for backwards compatibility and to support autoconfigure.

The ConfigXxxTests should verify that the current expectations still work.

The static JsonCreator should be a no go since there seems to be users with bizarre uses of Config, it's been a source of trouble for a while.

We tried to reduce complexity as much as possible here, but it's the legacy users that have made this impossible to simplify even more.

What kind of troubles are you facing? Note there's also SundrioConfig (plus ConfigBuilder and ConfigFluent) that must be changed accordingly for the complete builder functionality to work

[shawkins]

The multiple config constructors are there for backwards compatibility and to support autoconfigure.

Can you elaborate more the backwards compatibility? Since both need modified anytime a new config item is added, how are they to stay compatible? Do you mean for example that if there is a new config item added that there should be 4 constructors instead of 2?

What kind of troubles are you facing? Note there's also SundrioConfig (plus ConfigBuilder and ConfigFluent) that must be changed accordingly for the complete builder functionality to work

It relates to the last statement. We have two similar constructors. One with n parameters and one with n+1. If I am using the one with n+1 parameters, and the project adds new constructors to accomodate a new option, there will now either be:

• a constructor with n+1 and n+2 parameters
• or a constructor with n parameters, 2 constuctors with n+1 parameters, and a constructor with n+2 parameters

Depending on where in the parameter list the new thing is added there, and whether I'm using typed arguments, my usage of the old n+1 parameter constuctor may either be pointing to the wrong method or may be ambiguous.

I also don't see any path where we use a value for shouldSetDefaultValues other than true. Can you elaborate on how that is being used?

We tried to reduce complexity as much as possible here, but it's the legacy users that have made this impossible to simplify even more.

I can certainly decouple this change from this pr, but what's here now seems very difficult to maintain.

The static JsonCreator should be a no go since there seems to be users with bizarre uses of Config, it's been a source of trouble for a while.

Can you elaborate on what that would be? Are we supporting subclasses to Config where users want to have their own overridable constructor behavior?

Is there an issue / discussion where we could talk about longer run alternatives, such as using some kind of overridable init method method rather than constructors to handle some of this.

[manusa]

Can you elaborate more the backwards compatibility?

I can't recall exactly the scenarios.

Also, it seems that at some point the public constructor wasn't JavaDoced appropriately.

Users SHOULD NOT use the Config class constructor. They should rely on the Config builder instead or at most with the static Config creator methods.

Do you mean for example that if there is a new config item added that there should be 4 constructors instead of 2?

No, that shouldn't be the case anymore (that was the case in v6) -however the JavaDoc problem still remains 🤦-

At this point, the single constructor should be updated, the SundrioConfig, ConfigBuilder and ConfigFluent should be updated too.
The builder should then work as expected.

It relates to the last statement. We have two similar constructors.

We should only have a single public constructor for Config, all Config instances should be created via builders or static methods.

This is probably the confusing part, likely because I didn't update the JavaDoc 😓

I also don't see any path where we use a value for shouldSetDefaultValues other than true. Can you elaborate on how that is being used?

I really can't recall at the moment, there should definitely be a test to verify this.
I recall we had a problem when we needed to have a clean-slate config without the defaults, but I can't remember what were the scenarios.

Are we supporting subclasses to Config where users want to have their own overridable constructor behavior?

Yes, OpenShiftConfig is one example.

such as using some kind of overridable init method method rather than constructors to handle some of this.

new ConfigBuilder().withWhatever().build() should be the approach, Config should really remain hidden from the users.
The coupling with Sundrio and the years of legacy are what's making the class hard to maintain and complex.

[shawkins]

new ConfigBuilder().withWhatever().build() should be the approach, Config should really remain hidden from the users.
The coupling with Sundrio and the years of legacy are what's making the class hard to maintain and complex.

See if #7204 will work for you.

[manusa]

new ConfigBuilder().withWhatever().build() should be the approach, Config should really remain hidden from the users.
The coupling with Sundrio and the years of legacy are what's making the class hard to maintain and complex.

See if #7204 will work for you.

This seems to simplify things on the Config class by setting all the fields in the base Sundrio class.
For the classes that extend it I'm not sure we're making things simpler or more complicated.

From my side, I'm fine. It doesn't seem to break anything so feel free to mark it as ready and I'll merge

[shawkins]

@manusa rebased to pick up the config changes.