Creates spring-cloud-gcp-autoconfigure module #276
Conversation
Only contains the config module for now, remaining ones to come separately so that the PR doesn't grow too big. Also creates a spring-cloud-gcp-config module. Helps fixing #262
|
Will need to repeat for all the other starters. @snicoll FYI to make sure I'm on the right track. |
|
Travis doesn't like some your changes: |
|
Nevermind the point about config always working. Turns out I didn't maven reinstall everything, working as intended now. |
There was a problem hiding this comment.
I might miss something or just don't understand the whole picture, but here is what I feel on the matter.
I hope @snicoll will share some his wisdom as well when he has a spare cycle.
Thanks
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.cloud</groupId> | ||
| <artifactId>spring-cloud-gcp-starter-core</artifactId> |
There was a problem hiding this comment.
I think this must go. The new spring-cloud-gcp-autoconfigure should absorb its code fully.
| */ | ||
| @Configuration | ||
| @AutoConfigureAfter(GcpContextAutoConfiguration.class) | ||
| @EnableConfigurationProperties(GcpConfigProperties.class) | ||
| @ConditionalOnClass(GoogleConfigPropertySourceLocator.class) | ||
| @ConditionalOnMissingBean(GcpConfigProperties.class) |
There was a problem hiding this comment.
I think we don't need this condition. The @EnableConfigurationProperties does the trick anyway.
| org.springframework.cloud.bootstrap.BootstrapConfiguration=\ | ||
| org.springframework.cloud.gcp.config.autoconfig.GcpConfigAutoConfiguration,\ | ||
| org.springframework.cloud.gcp.autoconfigure.config.GcpConfigAutoConfiguration,\ |
There was a problem hiding this comment.
That's not BootstrapConfiguration part. This must be for the org.springframework.boot.autoconfigure.EnableAutoConfiguration property
There was a problem hiding this comment.
I think this is correct, because it's for BootstrapConfiguration, as required by Spring Cloud Config.
spring-cloud-gcp-config/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.cloud</groupId> | ||
| <artifactId>spring-cloud-gcp-starter-core</artifactId> |
There was a problem hiding this comment.
Non-Boot module can't depend on Boot. I see there GcpContextAutoConfiguration which should be as part of the spring-boot-autoconfigure as well and this one (spring-cloud-gcp-config) must be revisited respectivelly do not depend on Boot.
If we need this separate module at all though...
|
For some reason that I don't understand, the trace starter tests are stalling. |
|
Any ideas regarding current Travis failure? |
|
It seems something is adding There's a class load error and the Spring app gets into an endless loop through |
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.cloud</groupId> | ||
| <artifactId>spring-cloud-gcp-core</artifactId> |
There was a problem hiding this comment.
Is this dependency needed currently?
There was a problem hiding this comment.
Yeah, we need it to import GcpProperties, GcpProjectIdProvider, etc.
There was a problem hiding this comment.
Consider the case someone decides to add the auto-configuration of GCP to their stack but there are scenarios where they might not use it yet.
IMO, this should be <optional>true</optional> and you should have an extra condition to guard the fact this dependency is not available.
|
For me, a simple |
|
@meltsufin not sure why that's happening... Sounds like you have the wrong version of |
|
I found the conflict. <dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-configuration-processor</artifactId>
<optional>true</optional>
<exclusions>
<exclusion>
<groupId>com.vaadin.external.google</groupId>
<artifactId>android-json</artifactId>
</exclusion>
</exclusions>
</dependency>However, now the trace tests are stalling on: |
|
Sorry, I'll merge the fix shortly. |
Configuration now isn't auto-completing in STS, need to find out why.
|
@meltsufin it should be working now |
|
This change disables auto-completion for both |
|
The build still fails for me unless I exclude |
|
I wonder if this something with your local environment, since neither I nor Travis are seeing the same thing. |
spring-cloud-gcp-core/pom.xml
Outdated
| @@ -22,5 +22,9 @@ | |||
| <groupId>org.springframework</groupId> | |||
| <artifactId>spring-core</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.springframework.boot</groupId> | |||
| <artifactId>spring-boot-autoconfigure</artifactId> | |||
There was a problem hiding this comment.
Cyclic dependency?
Why should we worry about autoconfig here if it is just core?
Not sure what I am missing, but would like to take your attention on this.
Thanks
There was a problem hiding this comment.
spring-boot-autoconfigure, not spring-cloud-gcp-autoconfigure.
We need spring-boot-autoconfigure to get GcpProperties's @ConfigurationProperties annotation. LMK if I should be using another dependency.
There was a problem hiding this comment.
Oh! My bad. I see what is that.
Well, I think the problem that all the @ConfigurationProperties must be as a part of our autoconfigure not more.
Not sure how have you arranged it with Stephane, but I feel like something in architecture is still missing. Maybe there is still something in the target modules what is Boot-based? So, that should be moved to the autoconfigure module as well...
There was a problem hiding this comment.
Other spring-boot projects seem to be keeping their @ConfigurationProperties in the autoconfigure module as well.
I reviewed our project and all @ConfigurationProperties are used only within autoconfigure, except for the config and SQL ones.
For the config one, we might be able to make GoogleConfigPropertySourceLocator take in individual arguments instead of GcpConfigProperties (e.g., config name, enabled, etc.).
The SQL properties are used by the JdbcUrlProviders, but I think we can add those to the autoconfigure module as well..
There was a problem hiding this comment.
For the config one, we might be able to make
Are you going to do those changes to avoid this spring-boot-autoconfigure dependency in the core?
Or do you mean to do that in the separate issue?
Thanks
| org.springframework.cloud.gcp.config.autoconfig.GcpConfigAutoConfiguration,\ | ||
| org.springframework.cloud.gcp.core.autoconfig.GcpContextAutoConfiguration,\ | ||
| org.springframework.cloud.gcp.config.GoogleConfigPropertySourceLocator | ||
| org.springframework.cloud.gcp.config.GoogleConfigPropertySourceLocator |
There was a problem hiding this comment.
New line in the end of file.
The IDEA can be configured to do that automatically.
# Conflicts: # pom.xml
|
@snicoll PTAL I've added back the I'd prefer to keep |
|
I'm not sure I follow all of the discussion above (possibly refers to previous drafts), so at the risk of misunderstanding something, I have a couple of comments. We should look at it from the point of view of the user. As a user I want to use GCP as a config properties source (whatever that means). How do I achieve that? I expect it is: I add something to the classpath (one thing, or two?), and configure an env var (or equivalent). I believe @snicoll is offering to add a feature to start.spring.io so that even "two things on the classpath" can be specified as "one checkbox". So that would be nice. IMO we should make that happen, and reduce the number of tiny modules here as much as possible.
I don't understand that comment, and I don't understand why |
|
@dsyer Thank you for looking at this PR and your comments. We've gone back and forth on whether to have the On the topic of merging starters or exposing combination of them on start.spring.io, let's have that discussion in #300. |
| </dependency> | ||
| <dependency> | ||
| <groupId>commons-logging</groupId> | ||
| <artifactId>commons-logging</artifactId> |
There was a problem hiding this comment.
+1. Can't have commons-logging as a transitive for user apps. OTOH, if we just merge this tiny module and make it depend on Spring Boot (like it feels it should), then it's probably moot.
There was a problem hiding this comment.
Merger of the config module is being discussed in #299 (comment)
As per my comment, I don't think it's doable, but your input is welcome!
|
@artembilan Do you still have concerns on this PR? Can you do another quick pass please? |
|
@artembilan an NPE in our project? If so, I'd like to know about it. Maybe I can help. Also, provided @snicoll is OK with this PR, hope you don't mind if I dismiss your review. Comments were resolved, anyway. |
|
No, that's not related to this project. I have many others in my plate. OK. Feel free to dismiss and merge. I can review afterwards. |
I really would like to move forward with this and get a better idea on the bigger picture but unfortunately I can't. I feel we've been back and forth on this so I don't know what to do to unblock the situation. Here are some feedback on the current state, I hope this will help
Perhaps it makes sense to have a Having said that, I don't think I am giving any argument that I'ven't given already so feel free to move forward if you believe those objections aren't warranted. Thank you! |
meltsufin
dismissed
artembilan’s stale review
Review is now stale. Comments can be raised in the next PR that continues this work.
|
@meltsufin I don't understand why you would want to merge something that adds something that is discussed for removal in another issue (I find that super confusing). But if merging this PR helps moving things forward, sure thing. |
Only contains the config module for now, remaining ones to come
separately so that the PR doesn't grow too big.
Also creates a spring-cloud-gcp-config module.
Helps fixing #262