configure dedicated db user for database migrations: DML-only user fo… #29649
Conversation
…r datasource, but DDL user for migration #24639
|
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
|
|
||
| public Connection getConnection(String username, String password) | ||
| throws SQLException { | ||
| // TODO Auto-generated method stub |
There was a problem hiding this comment.
Please remove these comments
|
|
||
| if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent()) { | ||
| configure.dataSource( | ||
| new DataSourceDecorator(dataSource, flywayRuntimeConfig.username, flywayRuntimeConfig.password)); |
There was a problem hiding this comment.
You can invoke get() here and avoid using Optional in the class constructor
There was a problem hiding this comment.
I don't undertstand the usage of this pattern. I gave you good hints on how you can deal with this in your previous PR with some pseudo code that should allow you to do what you want and some advice on how to deal with the JDBC URL.
Not sure why you went another way? There might be a very good reason so don't hesitate to speak up.
There was a problem hiding this comment.
@gsmet Before I read your comments on the previous PR, I suggested @mfpc to use this pattern. The idea was to reuse the DataSource configuration and call DataSource#getConnection(String user, String password) when DataSource#getConnection() is called in the Flyway codebase.
But now I see that your suggestion seems nicer, so 👍🏻 to that
|
|
||
| import javax.sql.DataSource; | ||
|
|
||
| public class DataSourceDecorator implements DataSource { |
There was a problem hiding this comment.
The name doesn't really explain what this class does, but I am not very good at naming, so I'll let others decide about it
| var = rs.getInt(1); | ||
| } | ||
| } | ||
| assertEquals(100, var, "Different username and password was not executed"); |
There was a problem hiding this comment.
This test doesn't really verify the feature you provided
There was a problem hiding this comment.
This test if the connection is working with new configs.
There was a problem hiding this comment.
I will change the message for "Database connected with new config", its ok?
There was a problem hiding this comment.
I think you should rather get the datasource and check the user there. In your previous PR, I outlined how you could get the datasource URL. I'm pretty sure you can get the user from there too.
| return dataSource.isWrapperFor(arg0); | ||
| } | ||
|
|
||
| public <T> T unwrap(Class<T> arg0) throws SQLException { |
There was a problem hiding this comment.
Replace arg0 parameters with the name used in the sources
| @@ -43,6 +43,26 @@ public static FlywayDataSourceRuntimeConfig defaultConfig() { | |||
| @ConfigItem | |||
| public Optional<String> defaultSchema = Optional.empty(); | |||
|
|
|||
| /** | |||
| * This is a safety measurement against theoretical situations where the application goes haywire for any reason. | |||
| * This includes e.g. accidentally setting quarkus.hibernate-orm.database.generation to drop-and-create. | |||
There was a problem hiding this comment.
This description looks misleading and confusing
|
|
||
| /** | ||
| * Set the password for connection; | ||
| * quarkus.flyway.password=my_secret_password |
| * This includes e.g. accidentally setting quarkus.hibernate-orm.database.generation to drop-and-create. | ||
| * If the application runs with a restricted database user, damage in such situations can be mitigated. | ||
| * # Name of a custom database user for flyway to use. Must have DDL rights | ||
| * quarkus.flyway.username=my_schema_owner |
|
For the record, there are some worthy comments in the previous PR: #29628 |
| var = rs.getInt(1); | ||
| } | ||
| } | ||
| assertEquals(100, var, "Different username and password was not executed"); |
There was a problem hiding this comment.
I think you should rather get the datasource and check the user there. In your previous PR, I outlined how you could get the datasource URL. I'm pretty sure you can get the user from there too.
|
|
||
| if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent()) { | ||
| configure.dataSource( | ||
| new DataSourceDecorator(dataSource, flywayRuntimeConfig.username, flywayRuntimeConfig.password)); |
There was a problem hiding this comment.
I don't undertstand the usage of this pattern. I gave you good hints on how you can deal with this in your previous PR with some pseudo code that should allow you to do what you want and some advice on how to deal with the JDBC URL.
Not sure why you went another way? There might be a very good reason so don't hesitate to speak up.
|
#29704 seems to be a follow up to this. |
…r datasource, but DDL user for migration #24639