Skip to content

Conversation

@imonteroperez
Copy link
Contributor

  • Provides capabilities to override hardcoded forkCount value from mavenProperties option of plugin-compat-tester CLI

@imonteroperez imonteroperez marked this pull request as ready for review March 5, 2021 10:12
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Do you not need to adjust the CLI class?

For that matter, do you need this at all? Did you try simply

-mavenProperties forkCount=2

?

@imonteroperez
Copy link
Contributor Author

imonteroperez commented Mar 10, 2021

Do you not need to adjust the CLI class?

For that matter, do you need this at all? Did you try simply

-mavenProperties forkCount=2

?

No, I don't need to update the CLI class. Actually what motivated me to propose this PR is just because I tried what you said and it was not working as expected. I mean, if I use a -mavenProperties forkCount=2 as it is nowadays, it will launch PCT via CLI as follows:

[...]
running [/usr/bin/mvn, --show-version, [...], 
   --define=failIfNoTests=false, 
   --define=forkCount=2, [...] , 
   --define=forkCount=1, 
   hpi:resolve-test-dependencies, hpi:test-hpl, surefire:test, 
   -Djenkins.version=2.277.1-cb-2, -Denforcer.skip=true] 
[...]

So, how it its going to behave if I provide --define=forkCount=2 and PCT also includes --define=forkCount=1 ? Seems weird. I prefer to use 1 as default value in case you as user do not provide another forkCount.

@imonteroperez imonteroperez requested a review from jglick March 10, 2021 10:46
@jglick
Copy link
Member

jglick commented Mar 10, 2021

Rather than introducing a new option, you could

  • decline to do anything with forkCount if -mavenProperties already sets it
  • ensure that -mavenProperties come after the defaults, so they can override

@imonteroperez
Copy link
Contributor Author

  • decline to do anything with forkCount if -mavenProperties already sets it

That is exactly what I'm doing. No new option here :-)

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Seems to work.

@jglick
Copy link
Member

jglick commented Mar 11, 2021

Can someone merge this please, and at least update #261 with base branch? Would like to be able to close #275.

Also @oleg-nenashev would you like this repo to be converted to JEP-229 “CD”?

@imonteroperez imonteroperez merged commit bb4f15c into jenkinsci:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants