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.

Sign up for GitHub

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

Jump to bottom

Add a config option to enable Flyway baseline feature #27744

Merged
merged 1 commit into from
Sep 3, 2023
Merged

Add a config option to enable Flyway baseline feature #27744

merged 1 commit into from
Sep 3, 2023

Conversation

rmanibus
Copy link
Contributor

@rmanibus rmanibus commented Sep 6, 2022 •
edited by geoand
Loading

Resolves #27734

@quarkus-bot

This comment was marked as resolved.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand geoand changed the title flyway: config option to baseline new environment at startup Add a config option to enable Flyway baseline feature Sep 6, 2022
@geoand
Copy link
Contributor

geoand commented Sep 6, 2022 •
edited
Loading

Thanks for this.

It seems however that the test does not compile

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch 3 times, most recently from bf81cb4 to 909a763 Compare September 6, 2022 14:46
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 6, 2022

@geoand I was struggling to get the project imported correctly by intellij, it should be good now.

I also rephrased the baselineOnMigrate config doc because the current description is a bit misleading

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 909a763 to df6c6f3 Compare September 6, 2022 15:00
@quarkus-bot

This comment has been minimized.

Sign in to view
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 6, 2022

@geoand weird that FlywayExtensionMigrateAtStartTest is failing. H2 is not reset between tests ?

@geoand
Copy link
Contributor

geoand commented Sep 6, 2022

I'm pretty sure it's not

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from df6c6f3 to c4b8f2c Compare September 6, 2022 17:53
gastaldi
gastaldi reviewed Sep 7, 2022
View reviewed changes
...ns/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayDataSourceRuntimeConfig.java Outdated
*/
@ConfigItem
public boolean baselineOnMigrate;

/**
* true to execute Flyway baseline automatically when the application starts when executed against an empty schema, false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this command fails if the flyway_schema_history table exists (see #27734 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a test with an existing table to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gastaldi , I made some test and here is my conclusion:

if the flyway_schema_history table exist and contains any element, everything is ok.
if the flyway_schema_history does not exist, everything is ok.
if the flyway_schema_history exist but is empty, then flyway.info().current() is null so we do execute the baseline and it is failing.

I will try to find a way to check if flyway_schema_history already exist in flyway api.

@gastaldi
Copy link
Contributor

gastaldi commented Sep 7, 2022

@geoand weird that FlywayExtensionMigrateAtStartTest is failing. H2 is not reset between tests ?

Take a look at how it is done in https://github.com/quarkusio/quarkus/blob/main/extensions/flyway/deployment/src/test/java/io/quarkus/flyway/test/FlywayExtensionCallbackTest.java

@Ladicek
Copy link
Contributor

Ladicek commented Sep 7, 2022 •
edited
Loading

For the record, I'm running full Quarkus builds locally (completely unrelated to this PR) and I'm seeing FlywayExtensionMigrateAtStartTest fail as well. So I believe that's not caused by this PR, but sneaked in through some other PR.

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from aa1271d to 83e2549 Compare September 7, 2022 21:08
@quarkus-bot

This comment has been minimized.

Sign in to view
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 7, 2022

For the record, I'm running full Quarkus builds locally (completely unrelated to this PR) and I'm seeing FlywayExtensionMigrateAtStartTest fail as well. So I believe that's not caused by this PR, but sneaked in through some other PR.

@Ladicek I just changed the DB name in the connection string of my test and everything is working fine now

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 83e2549 to 7d0c739 Compare September 7, 2022 22:08
@rmanibus rmanibus requested a review from gastaldi September 8, 2022 12:37
@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 7d0c739 to 8a363d2 Compare September 9, 2022 15:10
@gsmet
Copy link
Member

gsmet commented Sep 13, 2022

@gastaldi can you have another look?

gastaldi
gastaldi requested changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments. I think it still needs some clarification

...ns/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayDataSourceRuntimeConfig.java Outdated Show resolved Hide resolved
...ns/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayDataSourceRuntimeConfig.java Outdated Show resolved Hide resolved
extensions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayRecorder.java Show resolved Hide resolved
@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 8a363d2 to ff0a056 Compare September 14, 2022 23:13
@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023
@geoand geoand removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Mar 4, 2023
@geoand
Copy link
Contributor

geoand commented Mar 4, 2023

Thanks!

Have the comments made during the review been addressed? cc @gastaldi

@rmanibus
Copy link
Contributor Author

rmanibus commented Mar 4, 2023

If I remember well I addressed all the concerns.
I could also include a guide on how to perform migrations squashing in the documentation.

gastaldi
gastaldi approved these changes Mar 4, 2023
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one minor observation

@quarkus-bot

This comment has been minimized.

Sign in to view
@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 7f3adde to 8aa8bde Compare March 4, 2023 23:09
@rmanibus
Copy link
Contributor Author

rmanibus commented Mar 4, 2023

If I remember well I addressed all the concerns. I could also include a guide on how to perform migrations squashing in the documentation.

fixed the typo. I will do this in another PR

@gastaldi
Copy link
Contributor

gastaldi commented Mar 4, 2023

Could you squash the commits before we trigger CI?

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 8aa8bde to 6d024e8 Compare March 6, 2023 13:00
@rmanibus
Copy link
Contributor Author

rmanibus commented Mar 6, 2023

@gastaldi I squashed it

@quarkus-bot

This comment has been minimized.

Sign in to view
@machi1990 machi1990 requested a review from gastaldi May 23, 2023 15:18
@Sanne
Copy link
Member

Sanne commented Aug 30, 2023

@gastaldi ping ? This looks like ready but it's waiting for your opinion

@gastaldi gastaldi force-pushed the flyway_baseline_at_start branch from 6d024e8 to 214fc34 Compare August 30, 2023 10:26
@gastaldi
Copy link
Contributor

@Sanne thanks, I've rebased the branch, it should be good to merge once it passes

@quarkus-bot

This comment has been minimized.

Sign in to view
@gastaldi
Copy link
Contributor

@rmanibus because I forgot to merge this PR 3 months ago, this now needs to be changed to match the latest Flyway codebase. Can you have a look?

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from 214fc34 to b2cded4 Compare August 30, 2023 22:57
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 3, 2023

@gastaldi it is rebased !

@gastaldi
Copy link
Contributor

gastaldi commented Sep 3, 2023

Can you rebase and remove the merge commit?

@rmanibus rmanibus force-pushed the flyway_baseline_at_start branch from e5b52f7 to 89cb68d Compare September 3, 2023 21:07
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 3, 2023

@gastaldi done !

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 3, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 3, 2023

✔️ 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.

@gastaldi gastaldi merged commit e3424be into quarkusio:main Sep 3, 2023
@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Sep 3, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi Awaiting requested review from gastaldi

Assignees
No one assigned
Labels
area/flyway kind/bugfix
Projects
None yet
Milestone
3.4.0.CR1
Development

Successfully merging this pull request may close these issues.

flyway baseline-version has no effect
6 participants
@rmanibus @geoand @gastaldi @Ladicek @gsmet @Sanne