[JENKINS-49488] implement any_met_condition flag for PromotionProcess.isMet() #115
Conversation
totoroliu
changed the title
totoroliu
changed the title
totoroliu
changed the title
…isMet() When any_met_condition flag is true, any met promotion condition would qualify for promote.
totoroliu
force-pushed
the
JENKINS-49488
branch
from
db9964d to
eadb5b2
Compare
|
any updates on this PR? |
|
@dwnusbaum |
| if(b==null) | ||
| return null; | ||
| badges.add(b); | ||
| LOGGER.log(Level.INFO, "PromotionProcess.isMet(): any_met_condition={0}", this.any_met_condition); |
There was a problem hiding this comment.
This logging does not help - PromotionProcess is not references in the entry
| /** | ||
| * Trigger promotion when any criteria condition is met. | ||
| */ | ||
| public boolean any_met_condition; |
There was a problem hiding this comment.
I would advice against introducing any more public fields in the plugin
There was a problem hiding this comment.
Can I use protected or private here?
I recall I hit an issue before when not using public.
|
yes, at beginning I was trying to use "Confitional Plugin" |
… for process-config.jelly to use.
| /** | ||
| * Trigger promotion when any criteria condition is met. | ||
| */ | ||
| private boolean any_met_condition; |
There was a problem hiding this comment.
nit: Java convention is to use camel case like anyMetCondition.
| return null; | ||
| badges.add(b); | ||
| if (this.any_met_condition) { | ||
| if (b!=null) |
There was a problem hiding this comment.
nit: Since you're modifying these two if statements anyway, I think it would be good to go ahead and use braces around the conditional statements.
| @@ -41,6 +41,11 @@ | |||
| </select> | |||
| </f:entry> | |||
|
|
|||
| <f:entry title="Any_Met"> | |||
There was a problem hiding this comment.
Is this what shows up in the UI? Maybe something like Promote if ANY of the above conditions are met would be more clear to users, but I'm not sure what would be best.
There was a problem hiding this comment.
The GUI looks like this:
Due to the alignment, I can't have long string at the title.
There was a problem hiding this comment.
Thanks for the screenshot! Maybe use Any Met with no underscore?
|
Do you have an existing test case that's testing multiple conditions with "AND"? |
|
@totoroliu I think SelfPromotionTest#testBasic uses 2 self promotion conditions, so maybe you could use that but change one of them into a ManualCondition that you never trigger to show your new option works. |
JENKINS-49488: implement any_met_condition flag for PromotionProcess.isMet()
When any_met_condition flag is true,
any met promotion condition would qualify for promote.