Simplify config data code a bit (CAN ONLY GO IN THE NEXT MAJOR RELEASE) #1189
Conversation
| bootstrapContext.registerIfAbsent(cls, BootstrapRegistry.InstanceSupplier.of(instance)); | ||
| bootstrapContext.addCloseListener(event -> event.getApplicationContext().getBeanFactory() | ||
| .registerSingleton(name, event.getBootstrapContext().get(cls))); | ||
| if (instance != null && !bootstrapContext.isRegistered(cls)) { |
There was a problem hiding this comment.
I've done two things here:
- check if
instancethat is passed is null, so that it slightly simplifies callers. - check if class is already registered with
!bootstrapContext.isRegistered(cls). We were already doing that viaregsiterIfAbsent, but than we were callingaddCloseListener, no matter if it was registered or not. Not a big deal, but this is cleaner, imo.
| * | ||
| * holds properties needed for ConfigData. | ||
| */ | ||
| record Properties(KubernetesClientProperties clientProperties, ConfigMapConfigProperties configMapProperties, |
There was a problem hiding this comment.
I wanted a class that deals with properties only, so that all properties are computed in a single place. The code has not changed at all, it was just moved to this class from other places and moved to a separate package.
| && secretsProperties.failFast(); | ||
| } | ||
|
|
||
| private KubernetesNamespaceProvider kubernetesNamespaceProvider(Environment environment) { |
There was a problem hiding this comment.
this was protected and overridden in both fabric8 and k8s-native clients, but the overridden version was exactly the same. Most probably an artifact of an initial thought process. I made it private here and dropped the methods in clients.
| @@ -49,11 +49,10 @@ public Fabric8ConfigDataLocationResolver(DeferredLogFactory factory) { | |||
|
|
|||
| @Override | |||
| protected void registerBeans(ConfigDataLocationResolverContext resolverContext, ConfigDataLocation location, | |||
| Profiles profiles, KubernetesConfigDataLocationResolver.PropertyHolder propertyHolder, | |||
There was a problem hiding this comment.
it used to be KubernetesConfigDataLocationResolver.PropertyHolder and now it is ConfigDataProperties
| ConfigDataLocationResolverContext resolverContext, ConfigDataLocation location, Profiles profiles) | ||
| throws ConfigDataLocationNotFoundException { | ||
|
|
||
| ConfigDataProperties properties = ConfigDataProperties.of(resolverContext).register(); |
There was a problem hiding this comment.
the logic in this class stays exactly the same, but now it's a lot shorter and easier to read. ConfigDataProperties.of(resolverContext).register() this both computed all 3 properties needed (client, configmap and secret ones) + registers them.
| public record ConfigDataProperties(KubernetesClientProperties clientProperties, | ||
| ConfigMapConfigProperties configMapProperties, SecretsConfigProperties secretsProperties) { | ||
|
|
||
| static Registrar of(ConfigDataLocationResolverContext context) { |
There was a problem hiding this comment.
the idea here is once you call of, the only option you have after is register (this is partial functions kind of), which will register the properties and give them back in form of the ConfigDataProperties instance. My sole purpose of the PR was around this thing, mainly, to decouple and make configdata slightly easier to reason about.
wind57
changed the title
| @@ -88,14 +86,6 @@ List<String> getAcceptedProfiles() { | |||
| return this.profiles.getAccepted(); | |||
| } | |||
|
|
|||
| public void setLog(Log log) { | |||
There was a problem hiding this comment.
we are not using these, so I decided to remove them
| return new KubernetesNamespaceProvider(environment); | ||
| } | ||
|
|
||
| private Environment environment(Map<String, Object> kubernetesConfigData, Profiles profiles) { |
There was a problem hiding this comment.
extract to a separate method, making resolveProfileSpecific even easier to read
No description provided.