-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[MNG-7511] Ensure the degreeOfConcurrency is a positive number in MavenExecutionRequest #767
[MNG-7511] Ensure the degreeOfConcurrency is a positive number in MavenExecutionRequest #767
Conversation
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 am not convinced by this. It should throw an IAE here. I would expect to fix the source.
To me, it seems like the safest approach. Another place to put the fix would be the MavenCli.calculateDegreeOfConcurrencyWithCoreMultiplier(...) method, but I like the protection in the config object more as it handles more cases without doing harm. Still, if you request the change, I can move the fix to the |
I think I have a better idea for this. First of all the arg need to checked whether it is a really an int or float. The C must be at the end. It must be larger than 0. The IAE still stands. When doing with float we could either use ceiling or floor. And then here set to one when smaller than 1. |
The C allowed only at the end would break the backward compatibility, see the MavenCliTest. So I've added a new commit with additional checks in the To be honest, the change from the |
a9592da
to
a2ed3bd
Compare
Picking this up...A breaking change in Maven 4 is fully acceptable. |
@kwart Added a commit on top:
I am not 100% happy because there are no methods to verify a true mathematical integer (whole number) and a real (fraction number). But is is likely good enough for now. WDYT? |
👍 LGTM |
…enExecutionRequest This closes apache#767
1929eb9
to
03b1faf
Compare
@kwart If you want this in Maven 3.9 please provide a followup PR. |
@michael-o Thanks for merging. WRT 3.9 backport: Should I care about the exception type and the |
Back port 1:1 because:
|
…enExecutionRequest This closes apache#767
…enExecutionRequest This closes apache#767
…enExecutionRequest This closes #767
JIRA: https://issues.apache.org/jira/browse/MNG-7511
This PR ensures the
degreeOfConcurrency
is always a positive number (defaulting to1
). It resolves issues when the computed value is< 1
.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.
[MNG-XXX] SUMMARY
, where you replaceMNG-XXX
and
SUMMARY
with the appropriate JIRA issue. Best practice is to use the JIRA issuetitle in the pull request title and in the first line of the commit message.
mvn clean verify
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.