Drop enable-api property (3) #2041
Conversation
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
| * @author Ioannis Canellos | ||
| * @author Isik Erhan | ||
| */ | ||
| @ConfigurationProperties(ConfigMapConfigProperties.PREFIX) |
There was a problem hiding this comment.
I am proposing to drop : @DefaultValue("true") boolean enableApi.
At the moment, we have two flags :
spring.cloud.kubernetes.config.enabled
spring.cloud.kubernetes.config.enable-api
With spring.cloud.kubernetes.config.enabled we control path support and also the creation of specific beans. With spring.cloud.kubernetes.config.enable-api we control if we are allowed to read configmaps via the api.
Since we have dropped paths support, we can have a single flag now for controlling everything, so we can drop spring.cloud.kubernetes.config.enable-api
Since users are supposed to control everything in this regard with a single flag : spring.cloud.kubernetes.config.enabled, they should be OK simply because @DefaultValue("true") boolean enabled.
For secrets though, @DefaultValue("false") boolean enabled, so they will need to switch this flag to true. Thus this a breaking change and had to wait for its turn.
There was a problem hiding this comment.
In other words the only option now is to use the API to load secrets and config maps since path support is no longer an option so the enabled flag covers all options, right?
There was a problem hiding this comment.
How did the secrets related tests work before with spring.cloud.kubernetes.secrets.enabled=false by default?
There was a problem hiding this comment.
In other words the only option now is to use the API to load secrets and config maps since path support is no longer an option so the enabled flag covers all options, right?
correct.
How did the secrets related tests work before with spring.cloud.kubernetes.secrets.enabled=false by default?
Until now, we were not setting that property at all in tests, and :
@Target({ ElementType.METHOD, ElementType.TYPE })
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Inherited
@ConditionalOnProperty(value = "spring.cloud.kubernetes.secrets.enabled", matchIfMissing = true)
public @interface ConditionalOnKubernetesSecretsEnabled {
}
Notice that matchIfMissing = true. So there was no need to set anything.
I have changed that to matchIfMissing = false in this PR. My thought process was that enableApi = false until now, so if you wanted to use the API to read properties, you had to enable that. And since we control that now via enabled, you still have to enable it specifically.
The other idea of why I decided to have it as matchIfMissing = false is because we are not supposed to read secrets by default, that is why they are secrets after all; it was natural for me when I was thinking about it.
There was a problem hiding this comment.
Ah makes sense, path support for secrets was on by default but api support was not, right?
| }); | ||
| } | ||
| Set<NormalizedSource> sources = new LinkedHashSet<>(this.properties.determineSources(environment)); | ||
| LOG.debug("Config Map normalized sources : " + sources); |
There was a problem hiding this comment.
the only thing that changed here is dropping :
if (this.properties.enableApi()) {
| }); | ||
| } | ||
| uniqueSources.forEach(secretSource -> { | ||
| MapPropertySource propertySource = getPropertySource(env, secretSource, properties.readType()); |
There was a problem hiding this comment.
same here, all I am doing is dropping the if in if (this.properties.enableApi()) {
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
|
@ryanjbaxter this one is ready and touches a lot of files, but they all stem from some simple ones, that I commented in the PR |
3 participants
No description provided.