Merge gcp-config module into gcp-autoconfigure

[meltsufin]

Spin-off from the discussion in #276.

[joaoandremartins]

Currently, the spring-cloud-gcp-config module exists so that we can enable Spring Cloud GCP Config conditionally, at the presence of GoogleConfigPropertySourceLocator.

The alternatives are:

1. Merge spring-cloud-gcp-config with spring-cloud-gcp-autoconfigure. We had it like this for a while, but it means that Spring Cloud GCP Config will always be enabled through spring-cloud-gcp-starter-core, which does not make sense since many spring-cloud-gcp users won't be interested in Spring Cloud GCP Config.

2. Merge spring-cloud-gcp-config with spring-cloud-gcp-starter-config. This would solve the issue of reducing the number of modules and enabling Spring Cloud GCP Config conditionally, at the cost of having code in the starter, which is an anti-pattern comparing to the Spring Boot standard.

We felt the separate spring-cloud-gcp-config module was the best compromise here. It follows the Spring Boot standard where the starter doesn't have any code and imports the generic autoconfigure module plus the service specific config module, even though the config by itself doesn't have any use.

If there are any other alternatives besides these that are better than what we currently have, feel free to suggest them. If not, it's probably safe to close this issue as working as intended.

[artembilan]

I think the main reason to have this spring-cloud-gcp-config as a separate artifact just because there is no external dependency which would allow us to auto-configure feature by the presence in classpath. In this case we may go the enable configuration property or just stay with the situation as it is right now. At least there are many samples outside of Spring Boot per se, where we have separate artifacts which are available by the starter afterwards.

[joaoandremartins]

In this case we may go the enable configuration property

We thought about this, but wouldn't it end up as a circular dependency?

i.e., GcpConfigAutoConfiguration enables GcpConfigProperties, but we'd need GcpConfigProperties to enable/disable GcpConfigAutoConfiguration (e.g., by using the enabled property).

[artembilan]

Not necessary that enabled property must be a part of the GcpConfigProperties.

The sample on the matter we can see in the Spring Cloud Sleuth: https://github.com/spring-cloud/spring-cloud-sleuth/blob/master/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceEnvironmentPostProcessor.java#L48

[meltsufin]

Relevant feedback from @snicoll:

spring-cloud-gcp-config shouldn't exist IMO:

• The GcpConfigPropertiesProvider doesn't look like an API you'd have designed without the constraints that have been expressed here. There is only one implementation in the auto-configuration module and I doubt anyone would bother implementing this interface to use this feature without Spring Boot
• The only reason it exists as far as I understand is to make sure the feature only kicks in when it is added to the classpath. But it doesn't have to be that way. If we want to be good citizen, we shouldn't enable this feature when running the code in our IDE. Perhaps we need a key, or we should check we are running on GCP.
• This module that, supposingly, isn't related to Spring Boot/Spring Cloud as a spring.factories in it. That doesn't sound right.

GcpConfigAutoConfiguration is empty. I can't help but think that something is missing there. What does this auto-configuration do exactly besides processing GcpConfigProperties? Why is it an auto-configuration if the thing that it auto-configures should run in the bootstrap context? I don't know how BootstrapConfiguration is supposed to work but I assume these are components that run in the parent context that processes the Environment. If I am right, that auto-configuration seems useless.

Perhaps it makes sense to have a spring-cloud-gcp-config module but I fail to see why considering that this feature should only be enabled (?) when the app run on GCP.

[snicoll]

Don't you need something more besides a class check? What happens if I have this on my classpath and I run the app in my IDE?

[joaoandremartins]

@snicoll it's not totally obvious to me that anything will be removed, which is why we're suggesting to merge the PR as-is.

We will need to address my comment and come up with a better alternative in order to remove anything.

We're aware of the limitations that exist, but some/most of them are a consequence of how Spring Cloud Config works. I made it clear before that this approach is the "lesser of the all evils" accessible to us.

GcpConfigAutoConfiguration is empty. I can't help but think that something is missing there.

It's the only way I know of using the @EnableConfigurationProperties annotation. If you have a better suggestion, let us know. Without it, we can't configure config.

Perhaps it makes sense to have a spring-cloud-gcp-config module but I fail to see why considering that this feature should only be enabled (?) when the app run on GCP.

There is no such restriction. You can use Config from outside GCP.

What happens if I have this on my classpath and I run the app in my IDE?

As far as I'm aware, in the unlikely scenario that anyone does that, nothing happens. spring-cloud-gcp-config's spring.factories only adds GoogleConfigPropertySourceLocator as a property source for bootstrap.

[joaoandremartins]

@artembilan are you suggesting we do something like @ConditionalOnProperty("spring.cloud.gcp.config.enabled", havingValue = "true")?

That could actually be a better compromise, if it works.

[joaoandremartins]

After playing around with this approach for a while, I found two considerable issues:

1. GoogleConfigPropertySourceLocator will always be in the classpath. It's statically in spring.factories, therefore we can't add it there conditionally. Bootstrap will always try to instantiate it and fail because GcpConfigProperties is disabled and no other GcpConfigPropertiesProvider bean exists in the context.

2. This change requires spring.cloud.gcp.config.enabled=true to be usable, since the default value would now be false. That's a usability hit worse than having an extra config module, in my opinion.

I don't think this is a viable solution due to these two points, but let me know if I'm missing anything.

[joaoandremartins]

What happens if I have this on my classpath and I run the app in my IDE?

I just tried running a new app with only spring-cloud-gcp-config in the classpath. It complains about the lack of a GcpProjectIdProvider bean for GoogleConfigPropertySourceLocator. I tried giving it the beans it needs with the @Bean annotation, but I don't think @Bean adds the beans to the bootstrap context, only to the application context.
Therefore, if there's a way for the user to add beans to the bootstrap context, I think it's fair game to allow the standalone use of the config module.

[artembilan]

@joaoandremartins ,

That was just an idea from an experience I have so far.

I'm not familiar with Spring Cloud Config project, but I guess that is a right place to borrow ideas how to be for this GCP Config feature.

[joaoandremartins]

Fixed by #341