Fix 1323 part 1 #1327
Fix 1323 part 1 #1327
Conversation
Configure Renovate
| /** | ||
| * Strategy that does nothing. | ||
| */ | ||
| public static final ConfigurationUpdateStrategy NOOP = new ConfigurationUpdateStrategy("no-op", () -> { |
There was a problem hiding this comment.
this is needed for configuration watcher, when reload is disabled. In that case, we do not care about a ConfigurationUpdateStrategy. The only reason we need it, is because we extend various classes that need it via the calls to super, but in the configuration watcher itself, we make no use of it.
I can go into the details here, if needed, just let me know.
| envVars.add(watcherDebug); | ||
|
|
||
| if (disableReload) { | ||
| V1EnvVar disableReloadEnvVar = new V1EnvVar().name("SPRING_CLOUD_KUBERNETES_RELOAD_ENABLED").value("FALSE"); |
There was a problem hiding this comment.
disable reload: SPRING_CLOUD_KUBERNETES_RELOAD_ENABLED - app still works. This test fails, without the fix that we did here.
I've also "tested" a build with disabling reload everywhere - that is, in the configuration watcher application.yaml and it passed all the integration tests.
|
@ryanjbaxter part-1 of the recent issue we discussed recently. I've separated these in two PRs (one coming later), because it is easier to review and debate if needed. Thank you for looking into it. |
| * @author wind57 | ||
| */ | ||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnProperty(value = "spring.cloud.kubernetes.reload.enabled", havingValue = "false", matchIfMissing = true) |
There was a problem hiding this comment.
when reload is disabled, bring in this dependency, otherwise the context does not start
There was a problem hiding this comment.
Do we really want matchIfMissing=true? ConfigReloadAutoConfiguration will be enabled in this case and we will end up with that auto configuration creating a ConfigurationUpdateStrategy. I guess this bean would not be created because there already by one, but we really only need this bean in the case where spring.cloud.kubernetes.reload.enabled=false so to me I would remove the matchIfMissing.
There was a problem hiding this comment.
Let's consider the case when spring.cloud.kubernetes.reload.enabled is simply not defined. We will have:
...
@ConditionalOnKubernetesReloadEnabled
....
public class ConfigReloadAutoConfiguration {
because ConditionalOnKubernetesReloadEnabled is defined as:
@Target({ ElementType.TYPE, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Inherited
@ConditionalOnProperty(name = "spring.cloud.kubernetes.reload.enabled", havingValue = "true")
public @interface ConditionalOnKubernetesReloadEnabled {
}
it will resolve to not match this conditional, as such it will not pick up this auto-configuration.
If we do not have matchIfMissing=true, it will not pick up the new auto-configuration either. no?
There was a problem hiding this comment.
Ah yes, sorry I was asumming ConditionalOnKubernetesReloadEnabled had matchIfMissing=true.
|
Is the second pr dependent on this one? |
|
no. the other issue is the fact that we react and schedule a reload event even if nothing changed in the config maps/secrets. They are totally independent, to me. |
|
OK let me know when the other PR is ready and we can merge these both together |
No description provided.