Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor #131

meltsufin · 2020-11-17T22:46:40Z

Fixes: spring-attic/spring-cloud-gcp#272

ttomsu · 2020-11-18T12:36:55Z

spring-cloud-gcp-autoconfigure/src/main/resources/META-INF/spring.factories

-org.springframework.cloud.gcp.autoconfigure.firestore.FirestoreRepositoriesAutoConfiguration,\
-org.springframework.cloud.gcp.autoconfigure.pubsub.health.PubSubHealthIndicatorAutoConfiguration,\
-org.springframework.cloud.gcp.autoconfigure.metrics.GcpStackdriverMetricsAutoConfiguration
+com.google.cloud.spring.autoconfigure.pubsub.GcpPubSubEmulatorAutoConfiguration,\


This seems like a big miss in my find/replace script, yet everything seems to work as expected. What does this file do?

You didn't miss it. It's actually fine. I just had issues reapplying changes that I initially started on the old repo to here. Look at the overall diff.

elefeint

Most of my comments are on code that got moved from one class to another, so not really blocking this specific PR. Still, any cleanup is welcome.

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

elefeint · 2020-11-19T20:50:32Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+		return null;
+	}
+
+	private boolean isOnClasspath(String className) {


Is there no Spring utility we can use for this?

I didn't find it. They do exactly the same thing in their @ConditionalOnClass implementation, but it's not reusable.

I don't know what version you looked at but OnClassCondition uses ClassUtils#isPresent that does what this method does.

I might've actually seen it somewhere else. ClassUtils#isPresent works, Thanks!

elefeint · 2020-11-19T20:51:33Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+
+	private CloudSqlJdbcInfoProvider buildCloudSqlJdbcInfoProvider(ConfigurableEnvironment environment, DatabaseType databaseType) {
+		CloudSqlJdbcInfoProvider cloudSqlJdbcInfoProvider = new DefaultCloudSqlJdbcInfoProvider(
+				getSqlProperty(environment, "database-name", null),


Should we be documenting these properties? Or at least enumerating them in some enum or constant set?

What's the benefit (for the enum or constant)?
I believe these props are already documented.

If we have to answer the question of "did we document everything", it would be easier to look in one spot and not play hide-and-seek with string constants. But it's up to you.

You're right. I added them.

elefeint · 2020-11-19T20:55:13Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+			String globalCredentialsLocation = environment.getProperty("spring.cloud.gcp.credentials.location", (String) null);
+			// First tries the SQL configuration credential.
+			if (sqlCredentialsLocation != null) {
+				credentialsLocationFile = application.getResourceLoader().getResource(sqlCredentialsLocation).getFile();


Can getResource() return null?

Javadoc says it's never null, but getFile() can throw an IOException which I guess is what we want anyway.

You can use Resource#exists or Resource#isFile.

I think we're fine with IOException here.

* Improve tests and documentation

...st/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessorTests.java

snicoll

Thanks for the ping @meltsufin, this looks much better. I've adde a few comments.

snicoll · 2020-11-20T05:22:02Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+
+	private boolean isOnClasspath(String className) {
+		try {
+			ClassUtils.forName(className, null);


You could use ClassUtils#isPresent rather.

snicoll · 2020-11-20T05:23:38Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+			String globalCredentialsLocation = environment.getProperty("spring.cloud.gcp.credentials.location", (String) null);
+			// First tries the SQL configuration credential.
+			if (sqlCredentialsLocation != null) {
+				credentialsLocationFile = application.getResourceLoader().getResource(sqlCredentialsLocation).getFile();


You can use Resource#exists or Resource#isFile.

snicoll · 2020-11-20T05:26:37Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

+		return null;
+	}
+
+	private boolean isOnClasspath(String className) {


I don't know what version you looked at but OnClassCondition uses ClassUtils#isPresent that does what this method does.

snicoll · 2020-11-20T05:29:26Z

...configure/src/main/java/com/google/cloud/spring/autoconfigure/sql/GcpCloudSqlProperties.java

- * @author Øystein Urdahl Hardeng
- */
-@ConfigurationProperties("spring.cloud.gcp.sql")
-public class GcpCloudSqlProperties {


I am confused. If you remove this class, no metadata is going to be generated for those keys anymore. If you hare exclusively using those in the EnvironmentPostProcessor you could keep this object and binds it programmatically using the Binder. If you don't, you need to generate the metadata manually so that those keys are still advertized in the IDE.

I didn't know about Binder and forgot about the metadata support. I'll look into it.

Can you please see if I did it right in the last commit? Thanks!

Thanks for the follow-up. The use of Binder looks correct to me.

…sor (GoogleCloudPlatform#131) * Sets Postgres default username: `postgres` * Improves tests and documentation for Cloud SQL Fixes: spring-attic/spring-cloud-gcp#272

meltsufin added 2 commits November 17, 2020 17:43

Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor

bc395bd

Fixes: spring-attic/spring-cloud-gcp#272

cleanup

e2c7b6e

ttomsu approved these changes Nov 18, 2020

View reviewed changes

Fix config it

21fa739

meltsufin force-pushed the sql-env branch from fe457fe to 21fa739 Compare November 18, 2020 14:35

some more cleanup and refactoring

3a1c21c

meltsufin marked this pull request as ready for review November 19, 2020 20:20

meltsufin requested review from elefeint, dmitry-s and dzou November 19, 2020 20:20

elefeint suggested changes Nov 19, 2020

View reviewed changes

elefeint approved these changes Nov 19, 2020

View reviewed changes

* Set Postgres default username: postgres

e38c40a

* Improve tests and documentation

meltsufin mentioned this pull request Nov 19, 2020

DataSourceProperties should not be changed by GcpCloudSqlAutoConfiguration spring-attic/spring-cloud-gcp#272

Closed

Merge branch 'master' into sql-env

359d147

elefeint reviewed Nov 19, 2020

View reviewed changes

...st/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessorTests.java Show resolved Hide resolved

snicoll reviewed Nov 20, 2020

View reviewed changes

meltsufin added 2 commits November 20, 2020 09:39

Merge branch 'master' into sql-env

ae8e2c4

Restore GcpCloudSqlProperties and use Binder

500ff58

meltsufin requested a review from elefeint November 20, 2020 19:33

elefeint approved these changes Nov 20, 2020

View reviewed changes

meltsufin merged commit 71d45f7 into master Nov 21, 2020

meltsufin deleted the sql-env branch November 21, 2020 01:14

Dattish mentioned this pull request Jan 20, 2021

spring.cloud.gcp.sql.enabled is ignored if specified in application.properties #215

Closed

meltsufin mentioned this pull request Feb 4, 2021

spring.cloud.gcp.sql properties getting ignored in application.yml #252

Closed

ElectroLutz mentioned this pull request Jun 7, 2021

No environment placeholder in configuration properties #491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor #131

Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor #131

meltsufin commented Nov 17, 2020

ttomsu Nov 18, 2020

meltsufin Nov 18, 2020

elefeint left a comment

elefeint Nov 19, 2020

meltsufin Nov 19, 2020

snicoll Nov 20, 2020

meltsufin Nov 20, 2020

elefeint Nov 19, 2020

meltsufin Nov 19, 2020 •

edited

Loading

elefeint Nov 19, 2020

meltsufin Nov 20, 2020

elefeint Nov 19, 2020

meltsufin Nov 19, 2020

snicoll Nov 20, 2020

meltsufin Nov 20, 2020

snicoll left a comment

snicoll Nov 20, 2020

snicoll Nov 20, 2020

snicoll Nov 20, 2020

snicoll Nov 20, 2020

meltsufin Nov 20, 2020

meltsufin Nov 20, 2020

snicoll Nov 20, 2020

Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor #131

Replaces CloudSqlAutoConfiguration with CloudSqlEnvironmentPostProcessor #131

Conversation

meltsufin commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin Nov 19, 2020 •

edited

Loading