Provide configuration properties for Flyway's Vault and Conjur support #25456
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
...ure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java
Show resolved
Hide resolved
...ure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java
Show resolved
Hide resolved
...ure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java
Show resolved
Hide resolved
...ure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java
Show resolved
Hide resolved
...ure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java
Outdated
Show resolved
Hide resolved
| @@ -328,6 +329,33 @@ | |||
| */ | |||
| private Boolean skipExecutingMigrations; | |||
|
|
|||
| /** | |||
| * The REST API URL of the Conjur server. Requires Flyway teams. | |||
There was a problem hiding this comment.
Configuration properties documentation don't start with "The" or "A". Please look at other description in this file (and others) for inspiration and this section of the reference guide.
| * path must start with the name of the engine and end with the name of the secret | ||
| * such 'kv/test/1/config'. Requires Flyway teams. | ||
| */ | ||
| private String vaultSecrets; |
There was a problem hiding this comment.
If that's a comma separated list, it can't be a String. We should offer a way for users to also use yaml list if they want to.
| "vaultSecrets"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
This shouldn't be flagged with @Test.
| } | ||
|
|
||
| @Test | ||
| private void assertThatPropertyResultsInFlywayTeamsUpgradeRequiredException(String propertyKeyAndValue, |
There was a problem hiding this comment.
I'd prefer that the setup remains in each test and that the thing that's shared is the real assertion in the form of a ContextConsumer.
A good example of what I am trying to convey is in EnvironmentEndpointAutoConfigurationTests.
snicoll
added
type: enhancement
|
@snicoll all comments addressed. |
|
|
||
| @Test | ||
| void vaultSecretsIsCorrectlyMapped() { | ||
| this.contextRunner.withUserConfiguration(EmbeddedDataSourceConfiguration.class) |
There was a problem hiding this comment.
@snicoll thx for the suggestion. Keeping the setup in each test makes it easy to see what the test is doing from first glance. Sharing only the common validation reduces the duplicated lines (and that was the main goal of the first attempt). 👍🏻
| @@ -108,9 +109,7 @@ void expectedPropertiesAreManaged() { | |||
| // Handled by the conversion service | |||
| ignoreProperties(configuration, "baselineVersionAsString", "encodingAsString", "locationsAsStrings", | |||
| "targetAsString"); | |||
| // Teams-only properties that we cannot detect as no exception is thrown and | |||
| // getters return null | |||
There was a problem hiding this comment.
This line should be deleted as well please.
There was a problem hiding this comment.
Yep, good catch @wilkinsona .
snicoll
changed the title
|
Thanks for the follow-up and for yet another great contribution Chris. |
Fixes gh-24969