-
Notifications
You must be signed in to change notification settings - Fork 134
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
[MRELEASE-1078] Support version policy to read and use SCM tags and commits #104
[MRELEASE-1078] Support version policy to read and use SCM tags and commits #104
Conversation
901c912
to
d99baa6
Compare
d99baa6
to
b39d474
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of changes which do not belong here, but are ultimately good to be applied. Can you move them into a separate PR? I'd be happy to review them.
|
b39d474
to
59ce8c0
Compare
Note:
|
59ce8c0
to
9c15a71
Compare
I have a question for the reviewers (@michael-o): |
What do you mean? Maven Release Plugin is a not a plugin on top of a plugin. Maven Release Plugin => Maven Release => Maven SCM API => Maven SCM Providers |
The Conventional Commits part has specific configuration. That "next version calculator" is a plugin for the maven-release plugin. |
I know see, it is not a plugin in Maven sense, since it is not a Maven plugin with mojos. You simply provide a new implementatino for an interface pluggable at runtime, no? |
Correct, this VersionPolicy is not a maven plugin, and as a consequence my editor (IntelliJ) does not support me in setting the right options.
Yes, and for this implementation there can be implementation specific configuration. I would like to offer settings configuration in the pom.xml of the project at hand. My question is simply: Is there a better way to do this?
|
There is an option to inject an |
Cool! Can you please point me towards this option on how to do this. Possibly even an example in another project? I would really like to have this in there. |
Good question. I don't know an example from the top of my head, but you can search |
Since this requires SCM 2, I'd recommend to try out the current snapshot of SCM 2 on top of this master. I know, that there will be failures... |
I take the clean master branch and only change in the main pom.xml
Then I run Then the Maven Release Manager fails with a lot of errors (70 failures). When looking through the errors I see these recurring ones:
@michael-o Are these the failures you were referring to? |
Correct.
That is the question whether this is just a DI issue.
Yes, absolutely. |
I checkout the master twice and did some debugging with both settings. |
9c15a71
to
bafce99
Compare
At this point my guess (I do not fully understand many of the details of the injection system used here) is that the root cause is related to some of the changes of #118 as I see changes in the ScmManagerStub parameters (like Priority) and also the PlexusJUnit4TestCase.java changes settings around the ScmManagerStub. @cstamas does this make sense? |
I have tried to figure out how this could work and I have not succeeded. So if you guys can point me to an example then that would be awesome. My main problem is that I do not understand how things are loaded. At this point I would put an extra method in the Any pointers towards making this possible is highly appreciated. |
@slawekjaranowski Can you help since you looked into XML-based mojo config for validation? |
How shall we proceed? Commit the way it is now (the config is an xml within a cdata block) or wait till the config has been changed to the way I would like it (where I am not yet able to do that)? |
@nielsbasjes One thing I know for sure is Wagon config in |
9b05e39
to
52870fb
Compare
Just did a rebase upon the latest master branch. |
@rfscholte @michael-o As an option that I'm fine with; how about I split this merge request into 2 parts:
This way the level of code you guys need to maintain is much smaller and thus this pull request becomes easier(?) for you to review. Just let me know what you prefer. I'm fine either way. |
This, I truly like. Extend and review API, then your implementation! |
Ok, then I'll change this in the coming weeks. |
Dank u! |
Taking things out is much simpler than making things :) I took out the version policy itself and the related documentation and comments. This is the cleanest I could think of. I have tested it by building my (out of project) VersionPolicy : https://github.com/nielsbasjes/conventional-commits-maven-release |
I'll add a very small test to ensure these new connection points for the version policies. |
903f4ff
to
e92ff95
Compare
Done. |
Note: This should really be squashed ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the actual implementation is now gone, the title of this PR as well as the JIRA issue should be renamed/rephrased. @nielsbasjes WDYT?
@michael-o Do you like this one? When merging this should be put in the squashed commit message also. |
This looks good, I'd fine to it: Support version policy to read and use SCM tags and commits WDYT? |
Updated. |
...e-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptorBuilder.java
Show resolved
Hide resolved
maven-release-api/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptor.java
Show resolved
Hide resolved
maven-release-api/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptor.java
Show resolved
Hide resolved
...elease-api/src/main/java/org/apache/maven/shared/release/versions/VersionParseException.java
Outdated
Show resolved
Hide resolved
...e-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptorBuilder.java
Show resolved
Hide resolved
maven-release-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseUtils.java
Show resolved
Hide resolved
...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java
Show resolved
Hide resolved
...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java
Outdated
Show resolved
Hide resolved
...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java
Outdated
Show resolved
Hide resolved
…ommits This closes apache#104
6af220b
to
b899ace
Compare
Thanks everyone. |
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles
,where you replace
MJAVADOC-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verify -Prun-its
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.
This my implementation on creating the option to follow the ideas of Conventional Commits to automatically determine the next version.
This patch assumes the required changes for maven-scm are available:
Summary of the changes:
projectVersionPolicyConfig
that contains the optional config for the version policy. NOTE: I have tried to make it a clean addition of the configuration for the maven-release-plugin but I have not been successful in adding the extra config schema to the plugin. So for now it is aString
which (because it is XML) must be included as CDATA.I'm really looking forward to your feedback on how I can improve this.