[JENKINS-62141] Allow more than one named exception to match each individual branch

[nfalco79]

Fix the branch named strategy where only properties defined in the first matching exception was returned. Now the strategy gather properties of all named exception that matches the current branch.

[nfalco79]

rebased

[nfalco79]

@dwnusbaum the same test failing in my closed PR are failing also here:

• TestTimedOutException
• EventsTest

[car-roll]

I think the issues were CI related. I created a test PR (#195) yesterday from one of your PRs because I thought there might be some errors with the actual workspace and finally got it to pass today just by trying again. Going to bump this PR to see if things pass now.

[nfalco79]

Good news!!

[dwnusbaum]

@nfalco79 Thanks for splitting this into a separate PR!

For other reviewers, here is an earlier discussion about this change: https://github.com/jenkinsci/branch-api-plugin/pull/160#discussion_r418672697.

[nfalco79]

Ok I will create an help file. Actually since there are no description normally I expect the sum of all matched expression.

what happens if more than one exception matches but those exceptions have different configurations for the same property

What do you prefer? Log a warning (if possible)? Build failure? In reals depends from the property. In my case I can add more ParameterDefinitionBranchProperty because the configuration of the property allow different behavior. Obviosly the NoTriggerBranchProperty or BuildRetentionBranchProperty is not the case so we can use the order of exception in the job definition supposing that equals and hash code can help to understand when 2 branch properties are considered the same.

[nfalco79]

the help text for NamedExceptionsBranchPropertyStrategy should be updated (I think by adding a help.html file here) to clarify

It does not work since neither jelly section neither jelly block supports help/description attribute. I had try to wrap <f:repeatable field="namedExceptions" add="${%Add exception}"> element with a jelly entry but it causes a UI misalignment between Default and Named. This is the best I can obtain with jelly description

[car-roll]

Looks like general CI issues, closing and reopening to restart

[dwnusbaum]

It does not work since neither jelly section neither jelly block supports help/description attribute

True, but you can add a help link manually using <f:helpLink>. Adding it to the Exceptions section seems to look ok (along with adding a help.html file):

diff --git a/src/main/resources/jenkins/branch/NamedExceptionsBranchPropertyStrategy/config.jelly b/src/main/resources/jenkins/branch/NamedExceptionsBranchPropertyStrategy/config.jelly
index 667eb82..433af5f 100644
--- a/src/main/resources/jenkins/branch/NamedExceptionsBranchPropertyStrategy/config.jelly
+++ b/src/main/resources/jenkins/branch/NamedExceptionsBranchPropertyStrategy/config.jelly
@@ -35,13 +35,13 @@
       </f:entry>
     </table>
   </f:repeatable>
+  <f:helpLink url="${descriptor.helpFile}" featureName="${%Exceptions}"/>
+  <f:helpArea/>

Screenshot (the far right help button is the new one):

[nfalco79]

I'm still a little worried about this causing problems for existing users.

Are you ok for this or not yet? Anyway please let you concentrate on #160 that is fundamental. NamedExceptionsBranchPropertyStrategy is an extension and could be implemented in a new specific plugin by us with the desiderate behavior, the #160 it is not

[nfalco79]

EDIT: This CI is a nightmare

[bitwiseman]

@nfalco79 Having finally had a chance to look at this, here's my perspective: Instead of changing the existing strategy, we should create a new strategy with the behavior your are looking for. Doing this will add some complexity (a new strategy) but is guaranteed not to break existing users. This will also let us define completely consistent behavior for how properties are applied and override each other in the case of multiple matches.

For example, instead of the way it is done now, we could start by applying the defaults and then override the default when branches match the a pattern. The patterns are evaluated in the order shown in the UI, overriding any previous property of the same type. The strategy would be called "Defaults with named overrides".

If you really want to keep the majority of the existing behavior, I'd still suggest creating a new strategy, called "Multi-match with Named Exceptions". This new strategy could extend the existing strategy with multi-matching. Again, this would be a safe change guaranteed not to break existing users, but which still gets the new behavior that you want.

As with #160, I think this is a great idea that will have huge value to a large number of users. I look forward to merging it when it is ready. If you want to chat about this, ping me on the gitter.im channel.

Thanks!

[nfalco79]

If you agree we can add a description to make clear the actual behaviour (was not clear that named exception are mutually exclusive).

I would make a point on but is guaranteed not to break existing users You can agree that it's strange users that define more exceptions that match a branch name and only the first one is applied they did not ask theirself why only one is applied and other no (at least this was my case).

Anyway I'm ok to create a new strategy.

• Defaults with named overrides: this strategy does not allow define a single distinct behaviour for a specific branch. For example if I have 5 branch patterns but one of them does not have to inherit the default strategy this implementation become useless (I shoudl repeat the common branch property into all named exception but one). Other point is overriding any previous property of the same type, branch property of same type (class) may have different behaviour dependening on settings. But this does not mean that same branch property are mutually exclusive. For example my branch property extends ParameterDefinitionBranchProperty that allow to add parameters for specific job (branch). So for example "master,support/*" I define 1 parameter and for "support/1.5.x" 1 parameter. This means that when I build the job support/1.5.x I will get 2 parameters, one from support/* and other one from support/1.5.x.
• Multi-match with Named Exceptions: all branch properties of matching strategies are applied. To understand if a property is the same of another one and should override (following the UI definition) we could implements a default method on BranchProperty interface equals, compare, isSameOf or whatever that say if two branch property are considered the same and than overrided is applied (this is the case of "Pipeline speed branch/duratibilit override")

[bitwiseman]

If you agree we can add a description to make clear the actual behaviour (was not clear that named exception are mutually exclusive).

I totally agree with this.

I would make a point on but is guaranteed not to break existing users You can agree that it's strange users that define more exceptions that match a branch name and only the first one is applied they did not ask theirself why only one is applied and other no (at least this was my case).

I also agree with this. The existing behavior seems very strange to me, but Jenkins users are good figuring out what what the current behavior is and then depend on it continuing to work the same way. 😄

Anyway I'm ok to create a new strategy.

• Defaults with named overrides: this strategy does not allow define a single distinct behaviour for a specific branch. For example if I have 5 branch patterns but one of them does not have to inherit the default strategy this implementation become useless (I shoudl repeat the common branch property into all named exception but one). Other point is overriding any previous property of the same type, branch property of same type (class) may have different behaviour dependening on settings. But this does not mean that same branch property are mutually exclusive. For example my branch property extends ParameterDefinitionBranchProperty that allow to add parameters for specific job (branch). So for example "master,support/" I define 1 parameter and for "support/1.5.x" 1 parameter. This means that when I build the job support/1.5.x I will get 2 parameters, one from support/ and other one from support/1.5.x.
• Multi-match with Named Exceptions: all branch properties of matching strategies are applied. To understand if a property is the same of another one and should override (following the UI definition) we could implements a default method on BranchProperty interface equals, compare, isSameOf or whatever that say if two branch property are considered the same and than overrided is applied (this is the case of "Pipeline speed branch/duratibilit override")

Hm, I don't completely understand what you're describing.

I think that only allowing one of each type/class of BranchProperty is simpler and easier, but I can also see that resulting in confusing behavior.

Let me use ParameterDefinitionBranchProperty as you suggested. That class contains a list of parameterDefinitions. If the ParameterDefinitionBranchProperty instances each have 1 ParameterDefinition and each ParameterDefinition has a different parameter name, then I agree with your result - As a user, I would expect that should result in two parameters. However, if there both ParameterDefinitions have the same name, as a user, I would expect only one should be be used. It is unclear to me that adding both ParameterDefinitionBranchProperty will result in this behavior.

As another example, if a BranchProperty has two checkboxes and the first one is checked in the first match and the second one is checked in the second match, is the functional result that they are both checked or both unchecked?

Maybe we need a mergeWith default method on BranchProperty merges two BranchProperty instances of the same type together?

[nfalco79]

As another example, if a BranchProperty has two checkboxes and the first one is checked in the first match and the second one is checked in the second match, is the functional result that they are both checked or both unchecked?

For this reason I simply propose other method like equals that say if the BranchProperty is the same and than will be overridden following the UI sequence or not. If not than both will be applied. Also a mergeWith could be usefull in case have sense for the BranchProperty (like parameters) but not for "Pipeline speed branch/duratibilit override" where I expect exception.

I mean for each BranchProperty of same class it should say: Is property A the same/equals to B? If yes than take B otherwise return B.mergeWith(A)

[nfalco79]

any news on this?