Skip to content

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
car-roll merged 9 commits into from
May 6, 2020
Merged

add symbol annotations to BranchProperty and BranchPropertyStrategy subclasses #192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b36838
add symbol annotations to BranchProperty and BranchPropertyStrategy s…
Apr 27, 2020
a2b5be9
renaming symbols
Apr 29, 2020
d9e1755
add symbol to RateLimitBranchProperty
Apr 30, 2020
b713225
update symbol name for UntrustedBranchProperty
May 1, 2020
21ac3a0
add dependency management
May 1, 2020
b10a28a
Revert "add dependency management"
May 1, 2020
bbef50a
bump parent pom and jenkins version
May 2, 2020
5e44032
make classes public to let mockito mock them
May 2, 2020
23607fc
use bom and disable resuse forks for surefire
May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/java/jenkins/branch/BuildRetentionBranchProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import hudson.model.Job;
import hudson.model.Run;
import jenkins.model.BuildDiscarder;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import java.io.IOException;
Expand Down Expand Up @@ -84,6 +85,7 @@ public Boolean run() {
/**
* Our {@link hudson.model.Descriptor}.
*/
@Symbol("buildRetention")
Copy link
Author

@car-roll car-roll Apr 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dwnusbaum dwnusbaum Apr 28, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Author

@car-roll car-roll Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@dwnusbaum dwnusbaum Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Author

@car-roll car-roll Apr 29, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@dwnusbaum dwnusbaum Apr 30, 2020

Choose a reason for hiding this comment

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

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.

@Extension
@SuppressWarnings("unused") // instantiated by Jenkins
public static class DescriptorImpl extends BranchPropertyDescriptor {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/jenkins/branch/DefaultBranchPropertyStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import jenkins.scm.api.SCMHead;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import java.util.ArrayList;
Expand Down Expand Up @@ -81,6 +82,7 @@ public List<BranchProperty> getPropertiesFor(SCMHead head) {
/**
* Our {@link BranchPropertyStrategyDescriptor}.
*/
@Symbol("allBranchesSameProperties")
Copy link
Member

@dwnusbaum dwnusbaum Apr 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Author

@car-roll car-roll Apr 28, 2020

Choose a reason for hiding this comment

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

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)

@Extension
@SuppressWarnings("unused") // by jenkins
public static class DescriptorImpl extends BranchPropertyStrategyDescriptor {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/jenkins/branch/NamedExceptionsBranchPropertyStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import jenkins.scm.api.SCMHead;
import org.apache.commons.lang.StringUtils;
import org.apache.tools.ant.types.selectors.SelectorUtils;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import java.io.File;
Expand Down Expand Up @@ -112,6 +113,7 @@ public List<BranchProperty> getPropertiesFor(SCMHead head) {
/**
* Our {@link BranchPropertyStrategyDescriptor}.
*/
@Symbol("namedBranchesDifferentProperties")
@Extension
@SuppressWarnings("unused") // by jenkins
public static class DescriptorImpl extends BranchPropertyStrategyDescriptor {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/jenkins/branch/NoTriggerBranchProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import hudson.model.Queue;
import hudson.model.Run;
import java.util.List;
import org.jenkinsci.Symbol;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.DataBoundConstructor;
Expand All @@ -52,6 +53,7 @@ public <P extends Job<P, B>, B extends Run<P, B>> JobDecorator<P, B> jobDecorato
return null;
}

@Symbol("suppressAutomaticTriggering")
@Extension
public static class DescriptorImpl extends BranchPropertyDescriptor {
@Override
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/jenkins/branch/UntrustedBranchProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import hudson.tasks.BuildWrapper;
import hudson.tasks.Publisher;
import java.lang.reflect.Type;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import java.util.ArrayList;
Expand Down Expand Up @@ -120,6 +121,7 @@ public <P extends Job<P,B>,B extends Run<P,B>> JobDecorator<P,B> jobDecorator(Cl
/**
* Our {@link Descriptor}.
*/
@Symbol("untrustedCode")
Copy link
Member

@dwnusbaum dwnusbaum Apr 28, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Author

@car-roll car-roll Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dwnusbaum dwnusbaum Apr 28, 2020

Choose a reason for hiding this comment

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

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

Copy link
Author

@car-roll car-roll Apr 28, 2020

Choose a reason for hiding this comment

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

Cool, I'll double check before I do anything

@Extension
@SuppressWarnings("unused") // instantiated by Jenkins
public static class DescriptorImpl extends BranchPropertyDescriptor {
Expand Down