chore: merge model parents and disconnect it from build parent#2323
chore: merge model parents and disconnect it from build parent#2323rsynek wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the model build by removing the layered “base parent” POM, folding its responsibilities into model-parent, and decoupling model-parent from the solver build parent to avoid cross-build side effects, while also updating renamed Enterprise dependencies.
Changes:
- Removed the
model-base-parentmodule from the model reactor and deleted its POM. - Made
timefold-solver-model-parentstandalone (no parent), adding its own key version/properties and BOM imports. - Updated Enterprise profile dependency/artifactId renames and added Enterprise BOM import/repository wiring.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| model/pom.xml | Removes facade/model-base-parent from the module list to reflect the parent POM merge. |
| model/facade/model-parent/pom.xml | Converts the model parent into a standalone parent POM, adds version management/BOM imports, and updates Enterprise profile dependencies. |
| model/facade/model-base-parent/pom.xml | Deletes the now-obsolete base parent POM after consolidation. |
rsynek
force-pushed
the
fix/fix-model-parent
branch
from
94ffa78 to
0aac227
Compare
|
winklerm
left a comment
winklerm
left a comment
There was a problem hiding this comment.
Thanks, the changes look good to me!
I have left a question and one concern about the flatten plugin, which I find potentially problematic, but I do not have any proof that it will not work without it, so leaving that up to your judgment.
| <relativePath>../model-base-parent/pom.xml</relativePath> | ||
| </parent> | ||
| <modelVersion>4.0.0</modelVersion> | ||
| <groupId>ai.timefold.solver</groupId> |
There was a problem hiding this comment.
Q: Is this the right groupId? I would expect something like ai.timefold.solver.model so that the artifacts are clearly distinguishable from the "core" solver artifacts, but fine if it was decided to name it this way.
There was a problem hiding this comment.
That has been already decided for all the artifacts to share the same groupId. The difference is in the artifactId prefix, which for the "sdk" artifacts contains model, like timefold-solver-model-definition, etc.
| <!-- Dependencies --> | ||
| <!-- ************************************************************************ --> | ||
| <version.ai.timefold.solver>${revision}</version.ai.timefold.solver> | ||
| <version.io.quarkus>3.35.3</version.io.quarkus> |
There was a problem hiding this comment.
PP: Would it make sense to add a comment to the build-parent mentioning that for instance Quarkus version is declared also in this file and should be kept in sync?
There was a problem hiding this comment.
FYI Dependabot happened earlier today. Some of these dependencies were upgraded.
| <modelVersion>4.0.0</modelVersion> | ||
| <groupId>ai.timefold.solver</groupId> | ||
| <artifactId>timefold-solver-model-parent</artifactId> | ||
| <version>${revision}</version> |
There was a problem hiding this comment.
I am concerned a bit that we are not using the maven-flatten-plugin because Maven docs explicitely says it has to be used when installing/deploying poms using properties like revision: https://maven.apache.org/guides/mini/guide-maven-ci-friendly.html#install-deploy
But I am not sure how to test that, apart from the fact that we try to build a model with this model parent (which already seems to work). I am wondering if we could get some pushback from Maven central when trying to deploy there..
| <ai.timefold.sdk.enterprise>true</ai.timefold.sdk.enterprise> | ||
| <!-- enable copy of Timefold diagnostic tools when building container image - see container profile --> | ||
| <ai.timefold.sdk.enterprise.skipTools>false</ai.timefold.sdk.enterprise.skipTools> |
There was a problem hiding this comment.
Is there an issue anywhere to refactor these properties to remove the "sdk" keyword?
Notable changes:
model-base-parentandmodel-parentas the idea of layered models is no longer relevantmodel-parentfrombuild-parentto avoid solver build process impact modelsenterpriseprofile