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

Mongodb Liquibase extension #18573

Merged
merged 1 commit into from
Aug 31, 2021
Merged

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Jul 9, 2021

Fixes #18009, #9801

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 9, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should count at least 2 words to describe the change properly

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file labels Jul 9, 2021
@loicmathieu loicmathieu changed the title Mongodb/liquibase Mongodb Liquibase extension Jul 9, 2021
@loicmathieu loicmathieu marked this pull request as ready for review July 13, 2021 10:57
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 13, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 3f65171

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Attach pull request number Upload pull request number ⚠️ Check → Logs Raw logs
CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 13, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 3f65171

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 16, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 3f65171

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 62d6459

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building a7dd23e

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Verify extension dependencies ⚠️ Check → Logs Raw logs

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Aug 20, 2021
@loicmathieu loicmathieu added area/liquibase release/noteworthy-feature and removed area/documentation area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Aug 20, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself.

@geoand
Copy link
Contributor

geoand commented Aug 23, 2021

Nice! I have a large backlog to catch up on, so I am not sure I'll get to this in the next 48 hours.

@loicmathieu
Copy link
Contributor Author

Nice! I have a large backlog to catch up on, so I am not sure I'll get to this in the next 48 hours.

Happy you're back and hope you have a good PTO.
It's not urgent, there is still plenty of time untill 2.3.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Seems pretty similar to the liquibase proper extension, so it looks good

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Aug 25, 2021
@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Aug 25, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 25, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building b77e500

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

Copy link
Member

@evanchooly evanchooly left a comment

Choose a reason for hiding this comment

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

Looks fine to me modulo a couple of style questions.

liquibase.database.jvm.JdbcConnection.class.getName()));

reflective.produce(new ReflectiveClassBuildItem(true, true, true,
liquibase.parser.ChangeLogParserCofiguration.class.getName(),
Copy link
Member

Choose a reason for hiding this comment

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

any reason for the FQCNs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as for your other 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.

Well, in fact if I use simple name this will ends up with tens of imports. I think that's why it has been done like this in the liquibase extension. Maybe using String would be better as it would avoid loading the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Howerver, when using String, you can ends up registering for reflection classes that didn't exist anymore as you'll not be notified when they'll deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe @geoand have an advice on this ?

Copy link
Member

Choose a reason for hiding this comment

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

personally, i'd rather have imports i can collapse away but don't hold up the merge just for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be just fine the way you have it.

@loicmathieu loicmathieu added this to the 2.3 - main milestone Aug 31, 2021
@loicmathieu loicmathieu merged commit 650761a into quarkusio:main Aug 31, 2021
@loicmathieu loicmathieu deleted the mongodb/liquibase branch August 31, 2021 15:22
@@ -159,6 +159,7 @@
<flyway.version>7.14.0</flyway.version>
<yasson.version>1.0.9</yasson.version>
<liquibase.version>4.4.3</liquibase.version>
<liquibase-mongodb.version>4.4.3</liquibase-mongodb.version>
Copy link
Member

Choose a reason for hiding this comment

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

A bit late to the party but is there a reason to have a separate version? Aren't they part of the same build?

Asking because it might be a pain for Dependabot.

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'll check, when I first start working on it month ago they had totally different versions, but the mongodb one was experimental.
If they are now always the same, I'll change it when I write the guide (before 2.3)

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks. And yes, I just think it needs a change if we are guaranteed they are built at the same time. What I'm worried about is Dependabot updating one at a time and thus having partial failing PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the version of the mongodb extension follow the one of the core liquibase but it's not release on the same day. I may have up to 10 days between a release of the core liquibase lib and the mongodb one.
There also can exist patch version for bugfixes on the liquibase mongodb extension (for ex 4.1.1.1 that don't exist for liquibase core).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/liquibase area/mongodb release/noteworthy-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquibase MongoDB extension
5 participants