add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

car-roll · 2020-04-27T22:56:12Z

adding symbol annotations to BranchProperty and BranchPropertyStrategy subclasses to make databinding easier

…ubclasses

car-roll · 2020-04-27T22:58:14Z

src/main/java/jenkins/branch/BuildRetentionBranchProperty.java

@@ -84,6 +85,7 @@ public Boolean run() {
    /**
     * Our {@link hudson.model.Descriptor}.
     */
+    @Symbol("buildRetention")


The DataBoundConstructor for this class includes BuildDiscarder as an argument. BuildDiscarder is an abstract class, so I was not sure if I needed to add Symbol annotation for BuildDiscarder subclasses? The same question applies to some other classes below.

Yes, ideally we would track down all implementations of BuildDiscarder and make sure they have symbols as well, but the only implementation I am aware of is LogRotator and that already has a symbol, see here.

That said, I don't think that BuildRetentionBranchProperty is intended to be directly configured by users because it doesn't show up in the UI when configuring a property strategy for a multibranch projects (not sure why, I don't see an isApplicable override in the descriptor), so maybe we shouldn't add a symbol for it?

I do see a jelly file for BuildRetentionBranchProperty here as well as UntrustedBranchProperty here
Or does having a jelly file not always mean there is a UI element?

@car-roll Yeah, I think they might show up in the UI in some context, I'm just not sure what those contexts are, because I don't see them as options when I configure a property strategy for a Multibranch Pipeline.

UntrustedBranchProperty implements BranchPropertyDescriptor.isApplicable so that it only shows up when the items inside of the MultiBranchProject extend hudson.model.Project, but WorkflowJob directly extends hudson.model.Job, so it makes sense that that one didn't show up in my testing.

After some digging it looks like Multibranch Pipelines have a DescriptorVisibilityFilter that suppresses BuildRetentionBranchProperty and RateLimitBranchProperty, so that explains why I didn't see them either in my testing.

So as far as I can tell, adding symbols to these types doesn't matter for Multibranch Pipelines, but we could still go ahead and add them for whatever projects do support them.

RateLimitBranchProperty already has a symbol annotation so we're good to go there.

Looks like the symbol is only for the descriptor for the job property, but I think we want one on the descriptor for the branch property.

car-roll · 2020-04-28T16:19:04Z

bouncing for CI

dwnusbaum · 2020-04-28T16:41:46Z

src/main/java/jenkins/branch/BuildRetentionBranchProperty.java

@@ -84,6 +85,7 @@ public Boolean run() {
    /**
     * Our {@link hudson.model.Descriptor}.
     */
+    @Symbol("buildRetention")


Yes, ideally we would track down all implementations of BuildDiscarder and make sure they have symbols as well, but the only implementation I am aware of is LogRotator and that already has a symbol, see here.

That said, I don't think that BuildRetentionBranchProperty is intended to be directly configured by users because it doesn't show up in the UI when configuring a property strategy for a multibranch projects (not sure why, I don't see an isApplicable override in the descriptor), so maybe we shouldn't add a symbol for it?

dwnusbaum · 2020-04-28T16:48:07Z

src/main/java/jenkins/branch/UntrustedBranchProperty.java

@@ -120,6 +121,7 @@ public UntrustedBranchProperty(String[] publisherWhitelist) {
    /**
     * Our {@link Descriptor}.
     */
+    @Symbol("untrustedCode")


I also don't see this show up as an option for Multibranch Pipelines, so IDK if we want a symbol or not. In this case, I think it's because Pipelines extend Job rather than Project, but I'm not sure.

If we do keep the symbol, maybe untrustedBranches would be better?

I'll remove it if it's not configurable. Probably best to keep it consistent with UI. Same goes with BuildRetentionBranchProperty that you mentioned in the above comment.

It might be configurable for some other implementation of MultiBranchProject, but I'm not sure what those might be.

Cool, I'll double check before I do anything

dwnusbaum · 2020-04-28T16:57:33Z

src/main/java/jenkins/branch/DefaultBranchPropertyStrategy.java

@@ -81,6 +82,7 @@ public DefaultBranchPropertyStrategy(@CheckForNull BranchProperty[] props) {
    /**
     * Our {@link BranchPropertyStrategyDescriptor}.
     */
+    @Symbol("allBranchesSameProperties")


Any thoughts on naming here (or for NamedExceptionsBranchPropertyStrategy)? I think I saw that you were considering sameProperties elsewhere? The display names for these types in the UI are "All branches get the same properties" and "Named branches get different properties", so I just tried to pick names that were close to what users would be used to from the UI. Maybe Properties is redundant since the type is BranchPropertyStrategy, so we could use allBranchesSame and namedBranchesDifferent or something? IDK what is best.

Yeah, I removed the suggestions because i thought i was overthinking it. But yeah, I thought the wording was a bit redundant. I'm fine with allBranchesSame and namedBranchesDifferent. I like to reduce names as much as possible, but I guess if you reduce it too much you can lose the meaning (like same and differentNames)

dwnusbaum · 2020-05-01T14:30:10Z

src/main/java/jenkins/branch/UntrustedBranchProperty.java

@@ -120,6 +121,7 @@ public UntrustedBranchProperty(String[] publisherWhitelist) {
    /**
     * Our {@link Descriptor}.
     */
+    @Symbol("untrustedBranches")


I should have thought about this more before I recommended untrustedBranches, but since the base type is BranchProperty I think the best symbol here is just untrusted.

car-roll · 2020-05-01T17:40:56Z

I'm not sure why there are dependency issues in CI and not locally, but going to try to address them

dwnusbaum · 2020-05-01T17:44:52Z

I'm not sure why there are dependency issues in CI and not locally, but going to try to address them

The problem is that the CI builds runs on multiple branches (recommendedConfigurations in the Jenkinsfile), and there was a recent update that changes the LTS version of one of those branches, see jenkins-infra/pipeline-library#92, and that version apparently changed some dependency versions leading to enforcer errors. You should see the same errors locally if you tried updating the Jenkins version to 2.222.3.

dwnusbaum · 2020-05-01T17:50:21Z

Looks like there is also a conflict for org.slf4j:slf4j-jdk14, the requested version in the new core version is 1.7.26 IIUC. I think the right way to fix these issues it to update to the newest version of the parent POM so that the core dependencies use the core BOM and then update the baseline here to 2.176.4, but I'm not sure if that would work.

car-roll · 2020-05-01T18:14:05Z

I'll give the update a shot

dwnusbaum · 2020-05-01T18:20:58Z

You might run into some Mockito problems once the core version is updated, see #160 which hit similar errors that you did and #184 which might fix those problems if you end up hitting them.

This reverts commit 21ac3a0.

car-roll · 2020-05-06T20:00:18Z

managed to resolve the issues using the BOM as well as setting reuseForks to `false. Thanks @dwnusbaum for the tip! This should resolve the problems in #194 and #160

add symbol annotations to BranchProperty and BranchPropertyStrategy s…

6b36838

…ubclasses

car-roll requested review from dwnusbaum and kshultzCB April 27, 2020 22:56

car-roll commented Apr 27, 2020

View reviewed changes

car-roll closed this Apr 28, 2020

car-roll reopened this Apr 28, 2020

dwnusbaum reviewed Apr 28, 2020

View reviewed changes

car-roll added 2 commits April 29, 2020 15:09

renaming symbols

a2b5be9

add symbol to RateLimitBranchProperty

d9e1755

dwnusbaum self-requested a review May 1, 2020 14:05

dwnusbaum reviewed May 1, 2020

View reviewed changes

car-roll added 2 commits May 1, 2020 08:58

update symbol name for UntrustedBranchProperty

b713225

add dependency management

21ac3a0

Revert "add dependency management"

b10a28a

This reverts commit 21ac3a0.

dwnusbaum mentioned this pull request May 1, 2020

[JENKINS-48837] Add BranchProperty support to OrganizationFolder #160

Merged

car-roll added 3 commits May 1, 2020 23:51

bump parent pom and jenkins version

bbef50a

make classes public to let mockito mock them

5e44032

use bom and disable resuse forks for surefire

23607fc

car-roll closed this May 6, 2020

car-roll reopened this May 6, 2020

car-roll requested a review from dwnusbaum May 6, 2020 19:58

dwnusbaum approved these changes May 6, 2020

View reviewed changes

dwnusbaum mentioned this pull request May 6, 2020

Fix mockito issues with JDK11 and jenkins 2.222.3 #194

Closed

car-roll merged commit 97f7edb into jenkinsci:master May 6, 2020

car-roll deleted the symbolize branch May 6, 2020 20:18

car-roll mentioned this pull request May 9, 2020

making inner classes public for mockito #184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

car-roll commented Apr 27, 2020

car-roll Apr 27, 2020

dwnusbaum Apr 28, 2020

car-roll Apr 28, 2020 •

edited

Loading

dwnusbaum Apr 29, 2020 •

edited

Loading

car-roll Apr 29, 2020

dwnusbaum Apr 30, 2020

car-roll commented Apr 28, 2020

dwnusbaum Apr 28, 2020

dwnusbaum Apr 28, 2020

car-roll Apr 28, 2020 •

edited

Loading

dwnusbaum Apr 28, 2020

car-roll Apr 28, 2020

dwnusbaum Apr 28, 2020

car-roll Apr 28, 2020

dwnusbaum May 1, 2020

car-roll commented May 1, 2020

dwnusbaum commented May 1, 2020 •

edited

Loading

dwnusbaum commented May 1, 2020

car-roll commented May 1, 2020

dwnusbaum commented May 1, 2020

car-roll commented May 6, 2020

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

Conversation

car-roll commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll commented May 1, 2020

dwnusbaum commented May 1, 2020 • edited Loading

dwnusbaum commented May 1, 2020

car-roll commented May 1, 2020

dwnusbaum commented May 1, 2020

car-roll commented May 6, 2020

car-roll Apr 28, 2020 •

edited

Loading

dwnusbaum Apr 29, 2020 •

edited

Loading

car-roll Apr 28, 2020 •

edited

Loading

dwnusbaum commented May 1, 2020 •

edited

Loading