[JENKINS-42849] add BranchProperty to assign ownership to branch jobs #63
Conversation
|
|
||
| @DataBoundConstructor | ||
| public BranchNameOwnershipStrategy(@RegEx String pattern, @Nonnull String ownerExpression) { | ||
| this.pattern = Pattern.compile(pattern); |
There was a problem hiding this comment.
This constructor is a part of public API, but it will throw runtime exception in the case of wrong regexp. It needs to be documented
| @Override | ||
| @Nonnull | ||
| public String getDisplayName() { | ||
| return "By branch name"; |
There was a problem hiding this comment.
Ideally it should be a localizable string coming from resources
| return "By branch name"; | ||
| } | ||
|
|
||
| public FormValidation doCheckPattern(@QueryParameter String value) { |
There was a problem hiding this comment.
Such API methods should be @Restricted(NoExternalUse.class)
| return FormValidation.ok(); | ||
| } | ||
|
|
||
| public FormValidation doCheckOwnerExpression(@QueryParameter String value) { |
| * @param branch The branch | ||
| * @return The prospective owner's user ID or full name. {@code null} if the owner cannot be determined. | ||
| */ | ||
| @Nullable |
There was a problem hiding this comment.
It should be @CheckForNull, Nullable does not enforce static checks
| import javax.annotation.Nonnull; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public class FromScmOwnershipStrategy extends BranchOwnershipStrategy { |
There was a problem hiding this comment.
FromScmBranchOwnershipStrategy, just to avoid possible confusion
| @Override | ||
| @Nonnull | ||
| public String getDisplayName() { | ||
| return "From scm"; |
| import com.synopsys.arc.jenkins.plugins.ownership.OwnershipDescription; | ||
| import com.synopsys.arc.jenkins.plugins.ownership.jobs.JobOwnerHelper; | ||
| import hudson.Extension; | ||
| import hudson.model.*; |
There was a problem hiding this comment.
Please avoid bulk imports in the main code part.
| Branch branch = multiBranchProject.getProjectFactory().getBranch(project); | ||
|
|
||
| String prospectiveOwner = strategy.determineOwner(branch); | ||
| String owner = prospectiveOwner != null ? prospectiveOwner : fallbackOwner; |
There was a problem hiding this comment.
So you do not try job/folder ownership?
There was a problem hiding this comment.
inherit the owner from multibranch job?
maybe we could have the fallback owner an optional setting. if unset, we fallback to inheritance of ownership according to global config.
The Is there a way to reuse |
|
Sorry, missed the update |
| @DataBoundConstructor | ||
| public BranchNameOwnershipStrategy(@RegEx String pattern, @Nonnull String ownerExpression) throws PatternSyntaxException { | ||
| this.pattern = Pattern.compile(pattern); | ||
| this.ownerExpression = ownerExpression; |
There was a problem hiding this comment.
fix empty and trim here as well? Then you can make this field @CheckForNull
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>branch-api</artifactId> | ||
| <version>2.0.8</version> | ||
| <type>jar</type> |
There was a problem hiding this comment.
Better to remove explicit jar requirement. It may become hpi at some point
| @Override | ||
| @Nullable | ||
| public String determineOwner(Branch branch) { | ||
| Matcher matcher = pattern.matcher(branch.getName()); |
There was a problem hiding this comment.
No PatternSyntaxException handling. It would be better to catch it here just in case (e.g. manual config edits on the disk)
| */ | ||
| public class BranchNameOwnershipStrategy extends BranchOwnershipStrategy { | ||
|
|
||
| private Pattern pattern; |
There was a problem hiding this comment.
Are you sure it will be persisted correctly and then recovered? I have some doubts about it, config roundtrip tests would be useful
| @DataBoundConstructor | ||
| public OwnershipBranchProperty(@Nonnull String fallbackOwner, @Nonnull BranchOwnershipStrategy strategy) { | ||
| this.strategy = strategy; | ||
| this.fallbackOwner = fallbackOwner; |
There was a problem hiding this comment.
fixEmptyAndTrim()? Null would be better
| try { | ||
| JobOwnerHelper.setOwnership(project, ownershipDescription); | ||
| } catch (IOException ioe) { | ||
| // TODO: handle somehow |
|
@jordancoll please fix it or close the PR. |
|
I would prefer to keep it open so that anybody could pick it up and finalize. |
No description provided.