GoogleCloudPlatform / spring-cloud-gcp

New home for Spring Cloud GCP development starting with version 2.0.
Apache License 2.0
415 stars 307 forks source link

Improve test experience in Cloud SQL integration #407

Open vy opened 3 years ago

vy commented 3 years ago

When using spring-cloud-gcp-starter-sql-{postgresql,mysql}, one needs to explicitly disable these and point Spring to a new datasource in application-test.properties:

spring.cloud.gcp.sql.enabled = false
spring.datasource.url = jdbc:postgresql://localhost:5432/app
spring.datasource.username = postgres

This experience can be improved by removing the need to explicitly disable Cloud SQL support during tests. In contrast, Pub/Sub only requires a single property, that is, spring.cloud.gcp.pubsub.emulator-host, to get going in tests. In the context of Cloud SQL, an alternative would be introduce a similar spring.cloud.gcp.sql.emulator-url property:

spring.cloud.gcp.sql.emulator-url = jdbc:postgresql://postgres@localhost:5432/app
meltsufin commented 3 years ago

Do you think it might be a bit misleading to call it emulator-url since we don't really have an emulator for Cloud SQL? What about just detecting if spring.datasource.url was provided, and effectively disabling CloudSqlEnvironmentPostProcessor based on that?

vy commented 3 years ago

I was also in doubt of using emulator-url due to the same concerns. Though wanted to keep the footprint similar to the one in Pub/Sub. Disabling CloudSqlEnvironmentPostProcessor if spring.datasource.url present sounds like a better approach. That said, I need to admit I am not sure if this might have some unwanted consequences. Thinking out loud... What if src/main has both spring.datasource.url and spring.cloud.gcp.sql configurations providing two DataSources: one pointing to a direct instance and one pointing to a GCP instance, respectively? Your proposal will not introduce a backward-incompatible change to this configuration, right?

meltsufin commented 3 years ago

We overwrite the spring.datasource.url anyway in CloudSqlEnvironmentPostProcessor. The only backwards incompatible behavior would be that we would stop overwriting it and potentially disable Cloud SQL config for those apps. So, it's still something we need to consider.