Don't set stupid defaults. #80
Conversation
I doubt any plugin cares about multicast dns discovery so just disable it rather than giving it a non standard port (which could lead to announcments on a public wifi network) ALso reuseForks is true by default so do not set it and allow users to use the standard -DreuseForks=false without having to change the plugin cofnig. On the same note concurrency is only ever used in surefires forkCount - yet another non standard property. So stop that nonsense and use forkCount (which is initialized to concurrency for backwards compatability)
jtnord
force-pushed
the
crazyness-must-die
branch
from
7da1d5e
to
6d85705
Compare
| @@ -59,7 +59,8 @@ | |||
| <!-- Change this property if you need your tests to be compiled with a different Java level than the plugin. --> | |||
| <!-- Use only if strictly necessary. It may cause problems in your IDE. --> | |||
| <java.level.test>${java.level}</java.level.test> | |||
| <concurrency>1</concurrency> <!-- may use e.g. 2C for 2 × (number of cores) --> | |||
| <concurrency>1</concurrency> <!-- DEPRECATED use forkCount --> | |||
There was a problem hiding this comment.
not sure we even need this but it here for backwards compatability... happy to remove both this and forkCount (as the default is one...)
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @@ -689,11 +690,9 @@ | |||
| <systemProperties> | |||
| <property> | |||
| <name>hudson.udp</name> | |||
| <value>33849</value> | |||
| <value>-1</value> | |||
There was a problem hiding this comment.
I mean why would we want this... like EVER? (if some esoteric plugin needs it for a test then let them configure it?)
There was a problem hiding this comment.
UDP disabling without throwing errors was only added in 1.514: jenkinsci/jenkins@19f34b5
This option is set at least since 1.388: https://github.com/jenkinsci/jenkins/blob/ea06f60c1abe408ccb45c700421c0456ce00ef6d/plugins/pom.xml#L196, probably a lot longer.
There was a problem hiding this comment.
@daniel-beck that's a novel use of "only" - something that happened more than 4 years ago :-)
do you care about that supporting building plugins for something as old as that?
There was a problem hiding this comment.
I'm just answering your comment. This didn't disable, because disabling didn't exist.
There was a problem hiding this comment.
Fine, though better to just move the code out of core, as is already filed in a split-plugins-from-core issue.
| @@ -689,11 +690,9 @@ | |||
| <systemProperties> | |||
| <property> | |||
| <name>hudson.udp</name> | |||
| <value>33849</value> | |||
| <value>-1</value> | |||
There was a problem hiding this comment.
Fine, though better to just move the code out of core, as is already filed in a split-plugins-from-core issue.
I doubt any plugin cares about multicast dns discovery so just disable
it rather than giving it a non standard port (which could lead to
announcments on a public wifi network)
Also
reuseForksis true by default so do not set it and allow users touse the standard
-DreuseForks=falsewithout having to change the pluginconfig.
On the same note
concurrencyis only ever used in surefiresforkCount-yet another non standard property. So stop that nonsense and use
forkCountdirectly (which is initialized to concurrency for backwardscompatability)
Tested on a plugin with an updated snapshot parent using
mvn test(check concurrency is one test at once)mvn test -Dconcurrency=4(check concurrency if 4 at once - for backwards comparability)mvn test -DforkCount=4(check concurrency is 4 at once)@reviewbybees