Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
8 changes: 7 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>cloudbees-folder</artifactId>
<version>5.1</version>
<version>6.0.1</version>
<optional>true</optional>
</dependency>
<dependency>
Expand Down Expand Up @@ -117,6 +117,12 @@
<version>4.3.4</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>branch-api</artifactId>
<version>2.0.8</version>
<type>jar</type>
Copy link
Member

Choose a reason for hiding this comment

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

Better to remove explicit jar requirement. It may become hpi at some point

</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package org.jenkinsci.plugins.ownership.model.branches;

import hudson.Extension;
import hudson.Util;
import hudson.model.User;
import hudson.util.FormValidation;
import jenkins.branch.Branch;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import javax.annotation.RegEx;
import java.util.Collections;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;

public class BranchNameOwnershipStrategy extends BranchOwnershipStrategy {

private Pattern pattern;
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it will be persisted correctly and then recovered? I have some doubts about it, config roundtrip tests would be useful

private String ownerExpression;

@DataBoundConstructor
public BranchNameOwnershipStrategy(@RegEx String pattern, @Nonnull String ownerExpression) {
this.pattern = Pattern.compile(pattern);
Copy link
Member

Choose a reason for hiding this comment

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

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

this.ownerExpression = ownerExpression;
Copy link
Member

Choose a reason for hiding this comment

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

fix empty and trim here as well? Then you can make this field @CheckForNull

}

public String getPattern() {
return pattern.pattern();
}

public String getOwnerExpression() {
return ownerExpression;
}

@Override
@Nullable
public String determineOwner(Branch branch) {
Matcher matcher = pattern.matcher(branch.getName());
Copy link
Member

Choose a reason for hiding this comment

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

No PatternSyntaxException handling. It would be better to catch it here just in case (e.g. manual config edits on the disk)


if (matcher.matches()) {
String prospectiveOwner = matcher.replaceAll(ownerExpression);

if (User.get(prospectiveOwner, false, Collections.emptyMap()) != null) {
return prospectiveOwner;
}
}

return null;
}

@Extension
public static class DescriptorImpl extends BranchOwnershipStrategy.BranchOwnershipStrategyDescriptor {

@Override
@Nonnull
public String getDisplayName() {
return "By branch name";
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it should be a localizable string coming from resources

}

public FormValidation doCheckPattern(@QueryParameter String value) {
Copy link
Member

Choose a reason for hiding this comment

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

Such API methods should be @Restricted(NoExternalUse.class)

try {
Pattern.compile(value);
} catch (PatternSyntaxException ex) {
return FormValidation.error("Invalid regex: %s", ex.getMessage());
}

return FormValidation.ok();
}

public FormValidation doCheckOwnerExpression(@QueryParameter String value) {
Copy link
Member

Choose a reason for hiding this comment

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

same

return Util.fixEmptyAndTrim(value) != null ? FormValidation.ok() : FormValidation.warning("Ownership will be disabled");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.jenkinsci.plugins.ownership.model.branches;

import hudson.ExtensionPoint;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import jenkins.branch.Branch;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

public abstract class BranchOwnershipStrategy extends AbstractDescribableImpl<BranchOwnershipStrategy> implements ExtensionPoint {

/**
* Determine the owner for the given branch using the implemented strategy.
*
* @param branch The branch
* @return The prospective owner's user ID or full name. {@code null} if the owner cannot be determined.
*/
@Nullable
Copy link
Member

Choose a reason for hiding this comment

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

It should be @CheckForNull, Nullable does not enforce static checks

public abstract String determineOwner(Branch branch);

@Nonnull
@SuppressWarnings("unchecked")
public BranchOwnershipStrategyDescriptor getDescriptor() {
return (BranchOwnershipStrategyDescriptor) super.getDescriptor();
}

static abstract class BranchOwnershipStrategyDescriptor extends Descriptor<BranchOwnershipStrategy> {

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.jenkinsci.plugins.ownership.model.branches;


import hudson.Extension;
import jenkins.branch.Branch;
import jenkins.scm.api.metadata.ContributorMetadataAction;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

public class FromScmOwnershipStrategy extends BranchOwnershipStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

FromScmBranchOwnershipStrategy, just to avoid possible confusion



@Nullable
@Override
public String determineOwner(Branch branch) {
ContributorMetadataAction contributorMetadataAction = branch.getAction(ContributorMetadataAction.class);
return contributorMetadataAction != null ? contributorMetadataAction.getContributor() : null;
}

@Extension
public static class DescriptorImpl extends BranchOwnershipStrategy.BranchOwnershipStrategyDescriptor {
@Override
@Nonnull
public String getDisplayName() {
return "From scm";
Copy link
Member

Choose a reason for hiding this comment

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

needs to be localizable

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package org.jenkinsci.plugins.ownership.model.branches;

import com.synopsys.arc.jenkins.plugins.ownership.OwnershipDescription;
import com.synopsys.arc.jenkins.plugins.ownership.jobs.JobOwnerHelper;
import hudson.Extension;
import hudson.model.*;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid bulk imports in the main code part.

import hudson.util.FormValidation;
import jenkins.branch.*;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;

import javax.annotation.Nonnull;
import java.io.IOException;
import java.util.Collections;

public class OwnershipBranchProperty extends BranchProperty {

private BranchOwnershipStrategy strategy;
private String fallbackOwner;

@DataBoundConstructor
public OwnershipBranchProperty(@Nonnull String fallbackOwner, @Nonnull BranchOwnershipStrategy strategy) {
this.strategy = strategy;
this.fallbackOwner = fallbackOwner;
Copy link
Member

Choose a reason for hiding this comment

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

fixEmptyAndTrim()? Null would be better

}

public BranchOwnershipStrategy getStrategy() {
return strategy;
}

public String getFallbackOwner() {
return fallbackOwner;
}

@Override
@SuppressWarnings("unchecked")
public <P extends Job<P, B>, B extends Run<P, B>> JobDecorator<P, B> jobDecorator(final Class<P> clazz) {
return new JobDecorator<P, B>() {
@Nonnull
public P project(@Nonnull P project) {
if (project.getParent() instanceof MultiBranchProject && TopLevelItem.class.isAssignableFrom(clazz)) {
MultiBranchProject multiBranchProject = (MultiBranchProject) project.getParent();
Branch branch = multiBranchProject.getProjectFactory().getBranch(project);

String prospectiveOwner = strategy.determineOwner(branch);
String owner = prospectiveOwner != null ? prospectiveOwner : fallbackOwner;
Copy link
Member

Choose a reason for hiding this comment

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

So you do not try job/folder ownership?

Copy link
Author

Choose a reason for hiding this comment

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

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.


OwnershipDescription ownershipDescription = new OwnershipDescription(true, owner, null);
try {
JobOwnerHelper.setOwnership(project, ownershipDescription);
} catch (IOException ioe) {
// TODO: handle somehow
Copy link
Member

Choose a reason for hiding this comment

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

handle what?

}
}
return project;
}
};
}

@Extension
public static class DescriptorImpl extends BranchPropertyDescriptor {
@Nonnull
@Override
public String getDisplayName() {
return "Set branch job ownership";
}

public FormValidation doCheckFallbackOwner(@QueryParameter String value) {
User user = User.get(value, false, Collections.emptyMap());
return user != null ? FormValidation.ok() : FormValidation.error("User '%s' is not registered in Jenkins", value);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:entry title="Capture pattern" field="pattern">
<f:textbox/>
</f:entry>
<f:entry title="Owner expression" field="ownerExpression">
<f:textbox/>
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<div>
The expression used to determine the owner.
May contain references to capture groups in the capture pattern using <i>$index</i> or <i>$name</i>.
See <a target="_blank" href="https://docs.oracle.com/javase/7/docs/api/java/util/regex/Matcher.html#appendReplacement(java.lang.StringBuffer,%20java.lang.String)">
java.util.regex.Matcher</a> for details.
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
A regular expression matching the branch name. Capture groups can be included and used in the owner expression.
See <a target="_blank" href="https://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html">java.util.regex.Pattern</a> for syntax.
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<div>
SCM sources can specify the "contributor" of a branch.
For example, GitHub and Bitbucket Branch Source plugins specify the user who opened a PR as its contributor.

This strategy sets this contributor as the owner of the branch job.
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:entry title="Fallback owner" field="fallbackOwner">
<f:textbox/>
</f:entry>

<f:dropdownDescriptorSelector field="strategy" title="Strategy" />
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<div>
If the chosen strategy cannot determine the owner, the fallback owner will be used.
</div>