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

Add a property to disable liquibase for selected data sources at build time #42225

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gcw-it
Copy link
Contributor

@gcw-it gcw-it commented Jul 30, 2024

Fixes #27437

This PR adds a build time configuration property quarkus.liquibase.active and accordingly quarkus.liquibase."named-data-sources".active.

It enables users to deactivate liquibase at build time for data sources which don't need migration.

The PR also contains some minor brush ups of existing code.

Are there any documentation tasks, that should be addressed in the context of this PR?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Nice patch! I think this looks a lot more like enabled than active even if it can be a bit blurry.

We ended up with the following convention:

  • enabled is build time and disables the feature entirely (can be per PU though)
  • active is runtime and you can use it to enable/disable the feature at runtime

Preventing the merge until we figure this out.

/cc @yrodiere to make sure it applies in this case as he came up with the convention :)

@yrodiere
Copy link
Member

/cc @yrodiere to make sure it applies in this case as he came up with the convention :)

I was actually writing a new issue about this, because while working on similar features I encountered inconsistencies in existing configuration. There may be a few things to discuss.

In the meantime, to allow this PR to move forward, I'd recommend the following:

  • If you only need to set this at build time, use enabled.
  • If you only want to disable "on-startup" behavior but still need to access Liquibase at runtime (like Flyway does with its active property) we'll need to discuss it.
  • If you also need to set this at runtime, use active but adapt the PR accordingly. And know that future changes in Quarkus may lead to Liquibase for a given datasource being completely unavailable at runtime if active=false.

@gcw-it
Copy link
Contributor Author

gcw-it commented Jul 31, 2024

The property needs to be evaluated at build time, to prevent the unsuccessful attempt to include non-existing changelogs in the native image.

Using enabled for this purpose would collide with the existing functionality, to disable liquibase altogether. In this case the extension should only be disabled for the default/named data source.

We could rename the property to something else, like enabled-for-data-source.

Please also see my comments in #27437.

@yrodiere
Copy link
Member

We could rename the property to something else, like enabled-for-data-source.

That's not a great name, but I wonder if we shouldn't rename the existing property to quarkus.liquibase.enable-extension, and do something similar to other extensions... Please don't act on this though, I'm writing up an issue and will post a link here as soon as I created it.

@yrodiere
Copy link
Member

yrodiere commented Jul 31, 2024

We could rename the property to something else, like enabled-for-data-source.

That's not a great name, but I wonder if we shouldn't rename the existing property to quarkus.liquibase.enable-extension, and do something similar to other extensions... Please don't act on this though, I'm writing up an issue and will post a link here as soon as I created it.

Wait a minute, we probably don't really have a conflict if we straighten things up. The existing quarkus.liquibase.enabled is actually a runtime property, and one that affects the whole extension instead of a particular datasource. This is also inconsistent with our conventions.

I would propose we straight up remove the existing quarkus.liquibase.enabled, and replace it with the per-datasource settings in this PR.

This would be a breaking change, but one that we need IMO. What worries me is the potential loss of data, so maybe we'll need a temporary workaround... We could for example:

  • fail on build if the default datasource is disabled, but named datasources are enabled
  • require applications to set some additional, temporary setting in that case (quarkus.liquibase.allow-disable-default-datasource=true).
  • remove the check and allow-disabled-default-datasource setting in a few versions (the one after the next LTS, most likely)

If a runtime setting is needed, we could introduce quarkus.liquibase.active, but make it per-datasource, this time. I don't see why it should affect all datasources, that was probably just a mistake.
Though I'm not sure even a per-datasource setting is needed. I'd argue that people can just set quarkus.liquibase.migrate-at-start=false/quarkus.liquibase.clean-at-start=false if that's what they want, it seems clearer to me.

@gsmet @gcw-it What do you think?

@gcw-it
Copy link
Contributor Author

gcw-it commented Jul 31, 2024

From a naming perspective this would be the ideal solution, albeit we'd need to make enabled a build time property.

There is the discrepancy between a jvm and a native build to keep in mind. The jvm build doesn't care if the resources (in this case change logs) are available at build time (probably because they could be set later via environment variable). The native build on the other hand fails if the resources aren't available to be included in the native image. That's why we need some possibility, to disregard chosen data sources at build time.

When choosing the route of repurposing the enabled flag, we could make it a triple value for a transitional period.

  • true could stay the same
  • ??? a value meaning, I really want to disable the default data source, I know what I'm doing (maybe call it force-disabled)
  • false would fail the build, with a message indicating to use the ??? value instead

After the transitional period false could regain its intended purpose and ??? could be deprecated for removal in a future version.

@yrodiere
Copy link
Member

I'm writing up an issue and will post a link here as soon as I created it.

See #42244

@gcw-it
Copy link
Contributor Author

gcw-it commented Jul 31, 2024

O.K. We'll proceed, when a consensus in #42244 arises.

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.

Failed to build native executable when liquibase is enabled on non-primary data source
3 participants