gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070907831
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -481,9 +481,9 @@ public Collection<Phase> phases() {
after(TEST_COMPILE),
after(TEST_RESOURCES),
dependencies(SCOPE_TEST,
READY))),
- phase(INTEGRATION_TEST)),
- phase(INSTALL, after(PACKAGE)),
- phase(DEPLOY, after(PACKAGE)))));
+ phase(INTEGRATION_TEST, after(BUILD))),
+ phase(INSTALL, after(VERIFY)),
+ phase(DEPLOY, after(VERIFY)))));
Review Comment:
Two separate concerns here — addressing each:
**Concern 1: No IT for DAG topology change.**
The `e2de348d` commit added `MavenITmng13230SkipPhasesTest` but it tests
skip-phases behavior, not the DAG topology. A dedicated IT verifying that `mvn
install` now implicitly runs the `verify` phase (i.e. a mojo bound to `verify`
executes) is still missing. This can be added as a separate test in the same IT
class or a new IT.
**Concern 2: #13232 is still open.**
This is a design question for @gnodet: is the DAG fix intentionally bundled
into this PR (in which case #13232 should be closed as superseded), or should
the DAG changes be kept separate? If separate, the `DefaultLifecycleRegistry`
hunk should be reverted from this PR and kept in #13232.
The skip-phases feature itself (the rest of the PR) is solid — only the DAG
bundling and the missing DAG IT remain unresolved.
_This review was generated by an AI agent, Hermès on behalf of @gnodet._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
