You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/07/30 22:04:37 UTC
[GitHub] [maven-release] michael-o opened a new pull request, #145: Simplify PerformReleaseMojo
michael-o opened a new pull request, #145:
URL: https://github.com/apache/maven-release/pull/145
@kwin @nielsbasjes Have a look whether this makes sense to you. I am not 100% certain about this. It feels awkward that this mojo does not extend from `AbstractScmReleaseMojo`, but still from `AbstractReleaseMojo` since other release mojos do so. If this isn't right a comment should be added to the code explaning why.
This affects #133 and #134.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] michael-o commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1221598456
@kwin @nielsbasjes @slavino I need a review to continue with other open issues.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] nielsbasjes commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1243336257
I'm wondering what the real downside (if any) is of having some properties available that are not used. Apparently (as shown in #149) some of those are actually missing and should be made available.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] nielsbasjes commented on a diff in pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on code in PR #145:
URL: https://github.com/apache/maven-release/pull/145#discussion_r950903889
##########
maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PerformReleaseMojo.java:
##########
@@ -179,16 +139,6 @@ public void execute()
releaseDescriptor.setScmSourceUrl( connectionUrl );
}
- if ( username != null )
- {
- releaseDescriptor.setScmUsername( username );
- }
-
- if ( password != null )
- {
- releaseDescriptor.setScmPassword( password );
- }
-
Review Comment:
Sure this should be removed?
Now the releaseDescriptor no longer has these values.
There is _no_ scenario when during the perform release the username/password are needed?
If there is no such scenario: Why do you keep the scm source url? I the ScmSourceUrl is needed then I would suspect the username and password are also needed in some cases.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] michael-o closed pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #145: Simplify PerformReleaseMojo
URL: https://github.com/apache/maven-release/pull/145
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] michael-o commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1200301826
@slavino Have a look also.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] kwin commented on a diff in pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #145:
URL: https://github.com/apache/maven-release/pull/145#discussion_r951087985
##########
maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PerformReleaseMojo.java:
##########
@@ -179,16 +139,6 @@ public void execute()
releaseDescriptor.setScmSourceUrl( connectionUrl );
}
- if ( username != null )
- {
- releaseDescriptor.setScmUsername( username );
- }
-
- if ( password != null )
- {
- releaseDescriptor.setScmPassword( password );
- }
-
Review Comment:
This is IMHO already part of `AbstractScmReleaseMojo.createReleaseDescriptor()`
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] nielsbasjes commented on a diff in pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on code in PR #145:
URL: https://github.com/apache/maven-release/pull/145#discussion_r951514243
##########
maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PerformReleaseMojo.java:
##########
@@ -179,16 +139,6 @@ public void execute()
releaseDescriptor.setScmSourceUrl( connectionUrl );
}
- if ( username != null )
- {
- releaseDescriptor.setScmUsername( username );
- }
-
- if ( password != null )
- {
- releaseDescriptor.setScmPassword( password );
- }
-
Review Comment:
@kwin You are right.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] kwin commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1221943545
I agree with @nielsbasjes that a lot more parameters are now exposed for goal `perform` which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between `AbstractScmReadReleaseMojo` and `AbstractScmWriteReleaseMojo` (inheriting from the former).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] edwardUL99 commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
edwardUL99 commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1243310807
> I agree with @nielsbasjes that a lot more parameters are now exposed for goal `perform` which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between `AbstractScmReadReleaseMojo` and `AbstractScmWriteReleaseMojo` (inheriting from the former).
As part of #149, this change could possibly be introduced? Unless @michael-o wishes to do that
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] nielsbasjes commented on a diff in pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on code in PR #145:
URL: https://github.com/apache/maven-release/pull/145#discussion_r950903477
##########
maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PerformReleaseMojo.java:
##########
@@ -179,16 +139,6 @@ public void execute()
releaseDescriptor.setScmSourceUrl( connectionUrl );
}
- if ( username != null )
- {
- releaseDescriptor.setScmUsername( username );
- }
-
- if ( password != null )
- {
- releaseDescriptor.setScmPassword( password );
- }
-
Review Comment:
Sure this should be removed?
There is no scenario when during the perform release the username/password are needed?
If there is no such scenario: Why do you keep the scm source url?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] nielsbasjes commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1221625797
I had a look at this code and I agree that there is indeed some duplication that can be simplified this way.
As the entire build pipeline passes I think the code change is good.
The only "effect" is that you reduce the code duplication (good) and the PerformReleaseMojo will have more config settings available than needed.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] kwin commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1243368619
For me the 2 phase release process is already complicated enough. Exposing unused parameters for the `perform` goal in its documentation (https://maven.apache.org/maven-release/maven-release-plugin/perform-mojo.html) will make it even harder to grasp what is going on. Also I expect a lot of wrong reports due to the unused (but documented) parameters.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-release] michael-o commented on pull request #145: Simplify PerformReleaseMojo
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #145:
URL: https://github.com/apache/maven-release/pull/145#issuecomment-1295903571
> I agree with @nielsbasjes that a lot more parameters are now exposed for goal `perform` which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between `AbstractScmReadReleaseMojo` and `AbstractScmWriteReleaseMojo` (inheriting from the former).
This totally makes sense.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org