Make a KuberentesServiceInstance a non-sealed class

[mipo256]

I want to bring up the question wether or not this is a good idea to make the KuberentesServiceInstance a sealed interface. Currently, it is. I do not insist at all, just opening the discussion on this question.

As far as I know there is no specification that undermines this class. On the other hand, it would be quite handy to be able to Mock this interface, since DefauktKuberentesServiceInstance is a Java record and can be quite an excessive it setup.

Any suggestions?

[wind57]

I made it sealed and tbh, I intend to make more things sealed. My reasoning is that when we want to change something in a public interface, we can't; because this would be a breaking change. If it s sealed, we would be allowed to do that.

I don't have a strong opinion for this, I am very open to suggestions. But, please do take into consideration, that we had pull requests that had to wait 3 years! until they could be merged, since they were with breaking changes.

Thank you for opening this issue and let's see where it goes.

[wind57]

@ryanjbaxter can you add the feedback label in here please?

[mipo256]

My reasoning is that when we want to change something in a public interface, we can't; because this would be a breaking change. If it s sealed, we would be allowed to do that.

@wind57 That is understandable, but IMVHO not very accurate. If you, for instance, remove the field from the public sealed record, then end-users are going to get a backward-incompatible change. Backward incompatible changes, as far as I see it, is something unavoidable, and sealed abstractions, although they limit the blast radius, they do not mitigate the change from being considered breaking.

So, you need major version bump anyway :)

[wind57]

You nailed it, it's not bullet proof, but it is one layer of protection.

Especially regarding your point about testing: will I agree making it non-sealed for the sake of testing? Most probably not, but than I'm only a contributor here, not an owner, so take my opinion with a gain of salt.

[ryanjbaxter]

@mipo256 can you provide us with an example of the test you are trying to right in which this is blocking you?

[mipo256]

Sure, @ryanjbaxter

We have an interface, that has an implementation, which is a pretty heavy object in terms of dependencies (it has like 7-10 other dependencies as beans), assume something like this:

public interface SomeContract {
    
    KubernetesServiceInstance getLastDiscoveredK8SService();
}

And we just want to mock this SomeContract#getLastDiscoveredK8SService. We do not want to bootstrap the real SomeContract implementation bean, for that we need to bootstrap the Spring Test context, provide the implementations for all the dependencies (maybe the @MockitoBean). That is a lot of additional code, plus we're turning unit test into integration test.

So, what we actually want, is to do something like:

KubernetesServiceInstance instance = <WHAT GOES HERE?>
Mockito.when(someContract.getLastDiscoveredK8SService()).thenReturn(instance);

Now, how do instantiate the KubernetesServiceInstance. It is a record. In order to instantiate it we need to provide all the parameters for it. We do not want that. We only care about the getInstanceId(). So the setup becomes quite excessive.

[wind57]

we are, internally, in the same situation in tests. And we create all the parameters for it.

I honestly don't see the problem here, create an internal utility that would only care about getInstanceId() and the rest of the fields would get some defaults.

May be I miss your point, but what I hear is : we want an easier way to test our code, so let's change the implementation. That, IMHO, is an argument too thin for such a change. But again, its just my opinion here, I'll wait for Ryan also.

[ryanjbaxter]

Thanks @mipo256!

So if I understanding you correctly your main issue here is that there are a lot of parameters needed to construct an instance of DefauktKuberentesServiceInstance and that makes your testing code more verbose because you can't just do

KubernetesServiceInstance instance  = mock(KubernetesServiceInstance.class);
when(instace.getInstanceId()).thenReturn("mockId");
when(someContract.getLastDiscoveredK8SService()).thenReturn(instance);

and instead you need to do

KubernetesServiceInstance instance  = new DefaultKubernetesServiceInstance("mockId", "test-svc-1", "2.2.2.2", 8080,
					Map.of("port.<unset>", "8080", "k8s_namespace", "namespace1", "type", "ClusterIP"), false,
					"namespace1", null);
when(someContract.getLastDiscoveredK8SService()).thenReturn(instance);

Is that correct?