-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Flyway Ignore Migration Patterns setting can't be set to an empty string #46984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Allows 'spring.flyway.ignore-migration-patterns' to be set to an empty string to align with Flyway's documented behavior for unsetting default migration patterns. Signed-off-by: Chanwon-Seo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've left a couple of comments for your consideration.
...way/src/main/java/org/springframework/boot/flyway/autoconfigure/FlywayAutoConfiguration.java
Show resolved
Hide resolved
...rc/test/java/org/springframework/boot/flyway/autoconfigure/FlywayAutoConfigurationTests.java
Show resolved
Hide resolved
...rc/test/java/org/springframework/boot/flyway/autoconfigure/FlywayAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
I think there are two options to consider for this issue: The first option is to leave the property in The second option is to initialize |
This is the better approach. Where possible, it's good for the default value of a Boot property to match the default value of whatever Boot's configuring. That benefits property auto-completion in an IDE, for example. For IDEs that use the metadata (IntelliJ no longer does, AFAIK), we may need to add some manual metadata as I can't remember if the annotation processor understands |
In that case the test can verify
It does work for I suggest add the following tests to cover all three scenarios: @Test
void ignoreMigrationPatternsCorrectlyMapped() {
//"spring.flyway.ignore-migration-patterns=*:missing,*:future"
}
@Test
void ignoreMigrationPatternsWhenEmpty() {
//spring.flyway.ignore-migration-patterns=
}
@Test
void ignoreMigrationPatternsUsesDefaultValuesWhenNotSet() {
//when not set, should be aligned with Flyway's new FluentConfiguration().getIgnoreMigrationPatterns()
}
P.S. Is it somehow possible to tell GitHub not to remove status: |
Not at the moment: spring-io/issue-bot#23. |
Thank you both, @wilkinsonaand @nosan, for the incredibly helpful and insightful discussion! I've updated the PR with all the suggested changes. I really appreciate the detailed guidance on aligning the properties with Flyway's defaults and the correct way to test for it. I've addressed it in ac59c8 |
Removes the `.whenNot(List::isEmpty)` condition to allow clearing the ignore-migration-patterns property via an empty string. To avoid affecting existing users, the property's default is now aligned with Flyway's default of `*:future` Signed-off-by: Chanwon-Seo <[email protected]>
Removes the `.whenNot(List::isEmpty)` check to allow clearing the property. The property's default is aligned with Flyway's to avoid breaking existing behavior, and new tests verify both changes. Signed-off-by: Chanwon-Seo <[email protected]>
...lyway/src/test/java/org/springframework/boot/flyway/autoconfigure/FlywayPropertiesTests.java
Outdated
Show resolved
Hide resolved
Reversed actual/expected comparison for ignoreMigrationPatterns, using type-safe ValidatePattern. Signed-off-by: Chanwon-Seo <[email protected]>
This PR resolves issue #46969.
Summary
Allows
spring.flyway.ignore-migration-patterns
to be set to an empty string to align with Flyway's documented behavior for unsetting default migration patterns.Changes
FlywayAutoConfiguration
, removed the condition that prevented an empty list from being passed to Flyway'signoreMigrationPatterns
configuration.spring.flyway.ignore-migration-patterns=
in application properties works as expected.Testing
The existing test
ignoreMigrationPatternsIsEmpty
inFlywayAutoConfigurationTests
was used to verify the fix.