Skip to content
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

Remove cleanOnValidationError config in Flyway #44912

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

gastaldi
Copy link
Contributor

@gastaldi gastaldi commented Dec 3, 2024

Starting in Flyway 11.0.0 (see https://documentation.red-gate.com/flyway/release-notes-and-older-versions/release-notes-for-flyway-engine), the cleanOnValidationError function and configuration has been removed.
An error will be thrown if this feature is configured.

Therefore it is better to remove this configuration from Quarkus so applications using it would fail as well (instead of deprecating it).

Starting in Flyway 11.0.0 (see https://documentation.red-gate.com/flyway/release-notes-and-older-versions/release-notes-for-flyway-engine), the `cleanOnValidationError` function and configuration has been removed.
An error will be thrown if this feature is configured.

Therefore it is better to remove this configuration from Quarkus so applications using it would fail as well (instead of deprecating it)
@gastaldi gastaldi requested review from geoand and gsmet December 3, 2024 23:57
gastaldi added a commit to quarkusio/quarkus-updates that referenced this pull request Dec 4, 2024
- This removes the `quarkus.flyway.clean-on-validation-error`. See quarkusio/quarkus#44912
Copy link

github-actions bot commented Dec 4, 2024

🎊 PR Preview 6a487f0 has been successfully built and deployed to https://quarkus-pr-main-44912-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link

quarkus-bot bot commented Dec 4, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 58f6144.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Dec 4, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 58f6144.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand
Copy link
Contributor

geoand commented Dec 4, 2024

That's weird as the code clearly still has the method and configuration... Nevermind I actually tried it and it fails when used

@geoand geoand merged commit 5006bf3 into quarkusio:main Dec 4, 2024
30 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 4, 2024
@gastaldi gastaldi deleted the flyway_deprecated branch December 4, 2024 11:01
gastaldi added a commit to quarkusio/quarkus-updates that referenced this pull request Dec 4, 2024
- This removes the `quarkus.flyway.clean-on-validation-error`. See quarkusio/quarkus#44912
@gsmet
Copy link
Member

gsmet commented Dec 4, 2024

I wonder if it's not a bit premature to drop the configuration property.

Quarkus won't fail if the property doesn't exist, it will just log a warning - which is easy to miss.

I wonder if we should deprecate the property instead, at least for a few versions and let it be a hard fail on the Flyway side. At least until they completely drop the method we are using (my understanding is that the code compiles but will throw an error for now).

@geoand
Copy link
Contributor

geoand commented Dec 4, 2024

What would be the benefit?

@gastaldi
Copy link
Contributor Author

gastaldi commented Dec 4, 2024

@gsmet I pondered about that, but because setting the configuration fails the app startup, I don't think it's safe to have it around anymore

@gsmet
Copy link
Member

gsmet commented Dec 9, 2024

Well, is it safer to completely ignore it and that the user is not aware at all that the config property is not doing anything anymore?

Because between this PR and the update recipe, the user has absolutely no chance to figure out this option is not available anymore.

My personal conviction is that we should at least have a release where we have the big failure before dropping it so that people have a chance to notice and adjust by themselves/find a workaround.

Now maybe I'm being too cautious and this config property is not critical enough but with data, you need to be extra safe.

@gastaldi
Copy link
Contributor Author

gastaldi commented Dec 9, 2024

Well, is it safer to completely ignore it and that the user is not aware at all that the config property is not doing anything anymore?

I am not sure, IMHO deprecating the config property would give a false illusion that the feature still works and the user can keep using it, which is no longer true.

Because between this PR and the update recipe, the user has absolutely no chance to figure out this option is not available anymore.

I am hoping that the Migration Guide would let them know about it: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.18#cleanonvalidationerror-configuration-removed

@geoand
Copy link
Contributor

geoand commented Dec 9, 2024

I am not sure, IMHO deprecating the config property would give a false illusion that the feature still works and the user can keep using it, which is no longer true.

That's my thinking as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants