JENKINS-31843 - Merge build parameter values when "Build is parameterized"

[Evildethow]

• Ensures no duplicates
• Give precedence to values supplied from "Build is parameterized" (overrides).

NOTE: Currently on rebuild if parameters are supplied via "Build is parameterized" then any pre existing parameters are clobbered.

[Evildethow]

@reviewbybees

[oleg-nenashev]

Needs several unit tests.
Please also avoid formatting changes, because they extend the review scope and may cause merge conflicts

[Evildethow]

@oleg-nenashev

• sry about the formatting. i missed that on review.
• will add unit tests

regards

[ghost]

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.

[Evildethow]

@oleg-nenashev Added tests

[amuniz]

You should check for null before accessing paramAction

[Evildethow]

@amuniz Added the null check to be safe but struggling to see how 'paramAction' would be null at this stage based on - https://github.com/jenkinsci/rebuild-plugin/blob/master/src/main/java/com/sonyericsson/rebuild/RebuildAction.java#L206

The lack of null checks in the existing code seems to echo this - https://github.com/jenkinsci/rebuild-plugin/blob/master/src/main/java/com/sonyericsson/rebuild/RebuildAction.java#L417

Either that or its just really fragile :)

cheers

[amuniz]

Probably you are right, the action will be always set and it's never null, but, in general, I think it's a good practice to check for null the return value of getAction, since it depends on someone adding that action before (and in some cases even in a separate plugin, which is clearly butterfly effect prone 😄 IMO)

[amuniz]

🐝

[varmenise]

I would suggest to rename this test, I think there's something wrong with the sentence itself and it is not really clear what's the goal of the test, probably something like "newParametersShouldOverrideExistingPatametersIfHaveSameName" or something like that.

[Evildethow]

changed

[varmenise]

🐜 IMHO the classes implemented to support the tests (IsCollectionContainingStringParameterValues, IsNotCollectionContainingStringParameterValues, StringParameterValueMatcher) are an overkill here and make it really difficult to follow the flow. You could just do all of that with a method which iterates on parameters without using the Matcher class. If you really want to stick with the approach, it should be a bit cleaned up (some methods/classes are unnecessary I would think)

[Evildethow]

@varmenise Cleaned up test names and refactored matchers into single matcher taking varargs. RE: "classes implemented to support tests are an overkill" - I would have to disagree when it comes to things like rules and matchers but as you mentioned this is possibly more personal preference / opinion than anything.

[varmenise]

Well, I actually meant that those 3 classes (that could be reduced to 1) were an overkill, but the current cleanup is good to me! so 🐝

[kwhetstone]

I think it's better to err on the side of "too many tests" just because you never know. It looks good with all the changes. 🐝

[Evildethow]

Well, I actually meant that those 3 classes (that could be reduced to 1) were an overkill

Sorry @varmenise I misread that. Blaming this on the early morning and weak coffee ;)

[varmenise]

@Evildethow no worries :)

[Evildethow]

@hagzag @shemeersulaiman Could i get a maintainer to look at this?