Skip to content

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Apr 8, 2025

Also adjusted a couple of other properties in passing.

This is part of an overall effort to make YAML configuration less confusing and avoid having to use ~.

@gsmet gsmet requested a review from yrodiere April 8, 2025 16:23
@quarkus-bot quarkus-bot bot added area/cache area/flyway area/gradle Gradle area/hibernate-orm Hibernate ORM area/hibernate-search Hibernate Search area/hibernate-reactive Hibernate Reactive area/infinispan Infinispan area/panache area/spring Issues relating to the Spring integration labels Apr 8, 2025
Copy link

quarkus-bot bot commented Apr 8, 2025

/cc @marko-bekhta (hibernate-search)

Copy link

github-actions bot commented Apr 8, 2025

🙈 The PR is closed and the preview is expired.

*
* Accepted values: `none`, `create`, `drop-and-create`, `drop`, `update`, `validate`.
*/
@WithParentName
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's generation.strategy in the properties files, do we need to keep the @WithParentName ?

@yrodiere
Copy link
Member

yrodiere commented Apr 9, 2025

So... I'm all for renaming, but if we're going that route, we might want to plan for the other renamings that we might need later?

See in particular this comment: #21866 (comment)

If I dared, I would suggest to go further and:

  • rename quarkus.hibernate-orm.database.generation to quarkus.hibernate-orm.schema-management.strategy. Because, you know, it's not just about "generation"! There's drop, too. Alternatively, I suppose quarkus.hibernate-orm.schema-management.action would work, too.

  • introduce a completely separate category with quarkus.hibernate-orm.data-management: quarkus.hibernate-orm.data-management.import-script, but also quarkus.hibernate-orm.data-management.strategy. The strategies would be none and import, the default being none in prod and import otherwise.

@gsmet gsmet force-pushed the rename-strategy branch from 6977507 to 96790b6 Compare April 11, 2025 08:01
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/vertx labels Apr 11, 2025
@gsmet gsmet changed the title Rename quarkus.hibernate-orm.generation to generation.strategy Rename quarkus.hibernate-orm.database.generation to quarkus.hibernate-orm.schema-management.strategy Apr 11, 2025
@gsmet gsmet force-pushed the rename-strategy branch from 96790b6 to 5b272ba Compare April 17, 2025 17:46
@gsmet gsmet marked this pull request as ready for review April 18, 2025 12:32
@gsmet
Copy link
Member Author

gsmet commented Apr 18, 2025

@yrodiere this is now ready!

This comment has been minimized.

Copy link
Contributor

@marko-bekhta marko-bekhta left a comment

Choose a reason for hiding this comment

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

This looks nice 🎉 !

only have that one question about using two properties at the same time (added inline)

Comment on lines +407 to +408
persistenceUnitConfig.database().generation().generation()
.orElse(persistenceUnitConfig.schemaManagement().strategy()));
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if the user applies both, then the deprecated option will "win"?
maybe we should check how the two match (or new one is none) and fail ? (if so, the same would apply to other options in this config)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we sure could. But I think it's going to be a bit annoying as you can't really know if the new config is set or not (if it's set to the default) if you don't make it optional.

And it makes dropping the old stuff a bit more painful.

But if you feel like it, feel free to build on it once this is in.

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM overall, but I think there's a small problem in dev service behavior...

Also make sure we use .strategy instead of a @WithParentName property.

This is part of an overall effort to make YAML configuration less
confusing and avoid having to use `~`.

Also related to:
quarkusio#21866 (comment)
@gsmet gsmet force-pushed the rename-strategy branch from 5b272ba to 18bd561 Compare April 25, 2025 11:59
@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 25, 2025
Copy link

quarkus-bot bot commented Apr 25, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 18bd561.

✅ 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.

This comment has been minimized.

Copy link

quarkus-bot bot commented Apr 28, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 18bd561.

✅ 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.

@yrodiere yrodiere merged commit 81f6bbc into quarkusio:main Apr 28, 2025
60 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.23 - main milestone Apr 28, 2025
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 28, 2025
@yrodiere
Copy link
Member

yrodiere commented Apr 28, 2025

@gsmet Merged, thanks! Will you add something to the migration guide (and maybe quarkus update)?

@lucamolteni
Copy link
Contributor

Quickstarts updated quarkusio/quarkus-quickstarts#1528

@gsmet
Copy link
Member Author

gsmet commented May 6, 2025

@lucamolteni awesome, that was on my TODO list.

I will prepare the entry in the migration guide and also the Quarkus Updates recipe.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants