Skip to content

[MNG-7353] support CLI "mvn pluginPrefix:version:goal"#757

Merged
hboutemy merged 1 commit intomaven-3.9.xfrom
MNG-7353
Jul 21, 2022
Merged

[MNG-7353] support CLI "mvn pluginPrefix:version:goal"#757
hboutemy merged 1 commit intomaven-3.9.xfrom
MNG-7353

Conversation

@hboutemy
Copy link
Copy Markdown
Member


String firstToken = tok.nextToken();
// groupId or pluginPrefix? heuristics: groupId contains . but not pluginPrefix
if ( firstToken.contains( "." ) )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we can make this assumption for sure since for testing or other reasong the groupId could be: foo, bar-baz

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if you want to test a plugin for such simplified run, just don't define such a groupId
at least, it seems to work for real world plugins, that's the first intent

of course, if you find a better heuristics, or even an algorithm, I'm all ears open

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I gave it some thought, but I can't find a better heuristic either. The only thing more or less foolproof would be to look at the first token, assuming it is a plugin prefix. If that lookup yields nothing, it would be a groupId.

Even that is not 100% safe, though. Imagine you're not connected to the network, so lookup fails, so we decide it's a groupId -- this certainly doesn't make sense. But more importantly, it's probably quite costly compared to a String.contains :-).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

and do you really think that we'll have any single word groupId in the future, given Maven Central explicitely asks for DNS matching?
heuristics is heuristics: in theory there could be edge cases, but in real world, it simply works

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This indeed breaks real-world projects. We're using a single word groupId internally and invoking maven using mvn groupId:artifactId:goal@executionId no longer works starting with Maven 3.8.8.

Copy link
Copy Markdown
Contributor

@mthmulders mthmulders left a comment

Choose a reason for hiding this comment

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

I like this feature a lot. Would it be possible to add an IT for it?

@michael-o
Copy link
Copy Markdown
Member

I like this feature a lot. Would it be possible to add an IT for it?

I second that.

@hboutemy
Copy link
Copy Markdown
Member Author

hboutemy commented Jul 3, 2022

IT added, that checks every CLI goal invocation format (with or without executionId)

mng7353 CliGoalInvocation.PrefixGoal()....................... OK (0.4 s)
mng7353 CliGoalInvocation.GroupIdArtifactIdVersionGoalAtId(). OK (0.3 s)
mng7353 CliGoalInvocation.PrefixVersionGoal()................ OK (0.3 s)
mng7353 CliGoalInvocation.GroupIdArtifactIdVersionGoal()..... OK (0.3 s)
mng7353 CliGoalInvocation.PrefixVersionGoalAtId()............ OK (0.3 s)
mng7353 CliGoalInvocation.PrefixGoalAtId()................... OK (0.3 s)
mng7353 CliGoalInvocation.GroupIdArtifactIdGoal()............ OK (0.3 s)
mng7353 CliGoalInvocation.GroupIdArtifactIdGoalAtId()........ OK (0.3 s)

please review
(notice: PR CI build does not seem to detect the associate core ITs branch...)

@mthmulders
Copy link
Copy Markdown
Contributor

Thanks for the IT. I've learned about mvn <pluginPrefix>:<goal>@<id> today :-)

Since there's no PR for the IT, let me ask one question here: I noticed the name of the test methods all start with test, but there's no @Test annotation. I thought we were on JUnit 4, with JUnit 5 underway. Wouldn't it make more sense to write the tests in JUnit 4 style? Also, if we were on JUnit 5 already, you could even turn it into a parameterised test, but I think that's not possible at the moment.

@mthmulders
Copy link
Copy Markdown
Contributor

(notice: PR CI build does not seem to detect the associate core ITs branch...)

Looking at the CI job for this PR, I see it detected the branch in m-i-t correctly and ran the new tests correctly:

image

@michael-o michael-o self-requested a review July 5, 2022 07:22
@michael-o
Copy link
Copy Markdown
Member

Will test this week...

@hboutemy
Copy link
Copy Markdown
Member Author

hboutemy commented Jul 5, 2022

on Junit 4 vs 5: I'm not a purist, I just copied from others, it's so basic :)

@michael-o michael-o self-requested a review July 21, 2022 19:18
Copy link
Copy Markdown
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

All good. I have already rebased and cleaned up for you. You just need to merge! Allez !

@hboutemy hboutemy merged commit 95bdbf6 into maven-3.9.x Jul 21, 2022
@hboutemy hboutemy deleted the MNG-7353 branch July 21, 2022 21:37
asfgit pushed a commit that referenced this pull request Jul 21, 2022
asfgit pushed a commit that referenced this pull request Feb 17, 2023
@jira-importer
Copy link
Copy Markdown

Resolve #8080

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants