Skip to content

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Feb 4, 2023

No description provided.

@@ -115,16 +115,6 @@ void inform() {
if (enableReloadFiltering) {
filter[0] = ConfigReloadProperties.RELOAD_LABEL_FILTER + "=true";
}

// We need to pass an APIClient to the SharedInformerFactory because if we use
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove this comment because the default constructor for SharedInformerFactory is deprecated anyway

@@ -1,3 +1,4 @@
org.springframework.cloud.kubernetes.client.discovery.catalog.KubernetesCatalogWatchAutoConfiguration
org.springframework.cloud.kubernetes.client.discovery.KubernetesInformerDiscoveryClientAutoConfiguration
org.springframework.cloud.kubernetes.client.discovery.KubernetesInformerAutoConfiguration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an auto-configuration

@@ -88,24 +85,4 @@ public KubernetesInformerReactiveDiscoveryClient kubernetesReactiveDiscoveryClie
serviceLister, endpointsLister, serviceInformer, endpointsInformer, properties);
}

@Bean
@ConditionalOnMissingBean
public CatalogSharedInformerFactory catalogSharedInformerFactory() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both CatalogSharedInformerFactory and SpringCloudKubernetesInformerFactoryProcessor are removed, as we do not use them anymore. They were using deprecated functionality anyway

@ConditionalOnCloudPlatform(CloudPlatform.KUBERNETES)
@AutoConfigureBefore({ SimpleDiscoveryClientAutoConfiguration.class, CommonsClientAutoConfiguration.class })
@AutoConfigureAfter({ KubernetesClientAutoConfiguration.class, KubernetesDiscoveryPropertiesAutoConfiguration.class })
public class KubernetesInformerAutoConfiguration {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "meat" of this PR is here. My main trigger towards making these changes was to drop deprecations we had around informers. The suggestion around deprecations was to create all you need manually, and don't rely on @KubernetesInformer annotations, for example. And this is exactly what I do here: create all the needed beans by hand. It creates the same beans as we did before, basically.

@Documented
@Inherited
@Conditional(ConditionalOnBlockingOrReactiveEnabled.OnBlockingOrReactiveEnabled.class)
public @interface ConditionalOnBlockingOrReactiveEnabled {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need this because informers related beans are only needed when either blocking or reactive clients are active

@wind57 wind57 changed the title User agent fix drop deprecations around informers + remove a few hacks we had where we were reading properties from Environment instead of properties Feb 5, 2023
return new Lister<>(endpointsSharedIndexInformer.getIndexer());
}

private String namespace(KubernetesDiscoveryProperties discoveryProperties,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic was verbatim taken as it was before in the previous code. It lacks support for "selective namespaces", but I will add it in a different PR

@wind57 wind57 marked this pull request as ready for review February 6, 2023 08:18
@wind57
Copy link
Contributor Author

wind57 commented Feb 6, 2023

@ryanjbaxter ready to be looked at. thank you.

@ryanjbaxter
Copy link
Contributor

There are a number of breaking changes in here so we are going to have to hold off on merging this

@wind57
Copy link
Contributor Author

wind57 commented Feb 7, 2023

My thought process was like this:

  • deleted classes are OK to be removed, they were made public only in the the previous merge that we had, thus they were not released and safe to be removed.
  • adjusting auto-configurations was done before by us in a "breaking manner", and no one should rely on those anyway.

I tend to agree with you a lot, cause you make sense, but I fail to see where the breaking change is in this PR :( please help me a bit. thank you!

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't realize that you removing some stuff that you recently created, but I still see some breaking changes I believe

@wind57 wind57 requested a review from ryanjbaxter February 7, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants