Skip to content

Commit

Permalink
[FIXED JENKINS-56658] BranchBuildStrategy implementations need to be …
Browse files Browse the repository at this point in the history 
…able to log rationale
  • Loading branch information
@stephenc
stephenc committed Mar 21, 2019
1 parent 86d7ef8 commit e930261
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 15 deletions.
2 changes: 1 addition & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.107.3</jenkins.version>
<java.level>8</java.level>
<scm-api.version>2.2.7</scm-api.version>
<scm-api.version>2.3.2</scm-api.version>
<git-plugin.version>3.3.0</git-plugin.version>
</properties>

Expand Down
71 changes: 68 additions & 3 deletions src/main/java/jenkins/branch/BranchBuildStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,14 @@
import hudson.ExtensionPoint;
import hudson.Util;
import hudson.model.AbstractDescribableImpl;
import hudson.model.TaskListener;
import hudson.util.LogTaskListener;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.scm.api.SCMHead;
import jenkins.scm.api.SCMRevision;
import jenkins.scm.api.SCMSource;
import jline.internal.Nullable;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.ProtectedExternally;

Expand Down Expand Up @@ -96,12 +101,41 @@ public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head
* @return {@code true} if and only if the {@link SCMRevision} should be automatically built when the
* {@link SCMHead} has been detected as created / modified.
* @since 2.0.17
* @deprecated use {@link #automaticBuild(SCMSource, SCMHead, SCMRevision, SCMRevision, TaskListener)}
*/
@Deprecated
@SuppressWarnings("deprecation")
@Restricted(ProtectedExternally.class)
public boolean isAutomaticBuild(@NonNull SCMSource source,
@NonNull SCMHead head,
@NonNull SCMRevision currRevision,
@CheckForNull SCMRevision prevRevision) {
throw new UnsupportedOperationException("Modern implementation accessed using legacy API method");
}

/**
* SPI: Should the specified {@link SCMRevision} of the {@link SCMHead} for the specified {@link SCMSource} be
* triggered when the {@link SCMHead} has been detected as created / modified?
*
* @param source the {@link SCMSource}
* @param head the {@link SCMHead}
* @param currRevision the {@link SCMRevision} that the head is now at
* @param prevRevision the {@link SCMRevision} that the head was last seen at or {@code null} if this is a newly
* discovered head. Care should be taken to consider the case of non
* {@link SCMRevision#isDeterministic()} previous revisions as polling for changes will have
* confirmed that there is a change between this and {@code currRevision} even if the two
* are equal.
* @param listener the {@link TaskListener} that can be used for outputting any rational for the decision
* @return {@code true} if and only if the {@link SCMRevision} should be automatically built when the
* {@link SCMHead} has been detected as created / modified.
* @since 2.1.3
*/
@Restricted(ProtectedExternally.class)
public abstract boolean isAutomaticBuild(@NonNull SCMSource source,
@NonNull SCMHead head,
@NonNull SCMRevision currRevision,
@CheckForNull SCMRevision prevRevision);
@CheckForNull SCMRevision prevRevision,
@NonNull TaskListener listener);

/**
* API: Should the specified {@link SCMRevision} of the {@link SCMHead} for the specified {@link SCMSource} be
Expand All @@ -124,9 +158,40 @@ public final boolean automaticBuild(@NonNull SCMSource source,
@NonNull SCMHead head,
@NonNull SCMRevision currRevision,
@CheckForNull SCMRevision prevRevision) {
return automaticBuild(source, head, currRevision, prevRevision,
new LogTaskListener(Logger.getLogger(getClass().getName()), Level.INFO));
}

/**
* API: Should the specified {@link SCMRevision} of the {@link SCMHead} for the specified {@link SCMSource} be
* triggered when the {@link SCMHead} has been detected as created / modified?
*
* @param source the {@link SCMSource}
* @param head the {@link SCMHead}
* @param currRevision the {@link SCMRevision} that the head is now at
* @param prevRevision the {@link SCMRevision} that the head was last seen at or {@code null} if this is a newly
* discovered head. Care should be taken to consider the case of non
* {@link SCMRevision#isDeterministic()} previous revisions as polling for changes will have
* confirmed that there is a change between this and {@code currRevision} even if the two
* are equal.
* @return {@code true} if and only if the {@link SCMRevision} should be automatically built when the
* {@link SCMHead} has been detected as created / modified.
* @since 2.1.3
*/
@SuppressWarnings("deprecation")
public final boolean automaticBuild(@NonNull SCMSource source,
@NonNull SCMHead head,
@NonNull SCMRevision currRevision,
@CheckForNull SCMRevision prevRevision,
@NonNull TaskListener listener) {
if (Util.isOverridden(BranchBuildStrategy.class, getClass(), "isAutomaticBuild", SCMSource.class,
SCMHead.class, SCMRevision.class, SCMRevision.class, TaskListener.class)) {
// modern implementation written to the 2.1.3+ spec
return isAutomaticBuild(source, head, currRevision, prevRevision, listener);
}
if (Util.isOverridden(BranchBuildStrategy.class, getClass(), "isAutomaticBuild", SCMSource.class,
SCMHead.class, SCMRevision.class, SCMRevision.class)) {
// modern implementation written to the 2.0.17+ spec
// legacy implementation written to the 2.0.17-2.1.2 spec
return isAutomaticBuild(source, head, currRevision, prevRevision);
}
if (Util.isOverridden(BranchBuildStrategy.class, getClass(), "isAutomaticBuild", SCMSource.class,
Expand All @@ -141,7 +206,7 @@ public final boolean automaticBuild(@NonNull SCMSource source,
}
// this is going to throw an abstract method exception, but we should never get here as all implementations
// have to at least have overridden one of the methods above.
return isAutomaticBuild(source, head, currRevision, prevRevision);
return isAutomaticBuild(source, head, currRevision, prevRevision, listener);
}

/**
Expand Down
13 changes: 7 additions & 6 deletions src/main/java/jenkins/branch/MultiBranchProject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2057,7 +2057,7 @@ public void observe(@NonNull SCMHead head, @NonNull SCMRevision revision) throws
// the previous "revision" for this head is not a revision for the current source
// either because the head was removed and then recreated, or because the head
// was taken over by a different source, thus the previous revision is null
if (isAutomaticBuild(source, head, revision, null)) {
if (isAutomaticBuild(source, head, revision, null, listener)) {
scheduleBuild(
_factory,
project,
Expand All @@ -2076,7 +2076,7 @@ public void observe(@NonNull SCMHead head, @NonNull SCMRevision revision) throws
listener.getLogger()
.format("Changes detected: %s (%s → %s)%n", rawName, prevRevision, revision);
needSave = true;
if (isAutomaticBuild(source, head, revision, prevRevision)) {
if (isAutomaticBuild(source, head, revision, prevRevision, listener)) {
scheduleBuild(
_factory,
project,
Expand Down Expand Up @@ -2110,7 +2110,7 @@ public void observe(@NonNull SCMHead head, @NonNull SCMRevision revision) throws
needSave = true;
// get the previous revision
SCMRevision prevRevision = _factory.getRevision(project);
if (isAutomaticBuild(source, head, revision, prevRevision)) {
if (isAutomaticBuild(source, head, revision, prevRevision, listener)) {
scheduleBuild(
_factory,
project,
Expand Down Expand Up @@ -2174,7 +2174,7 @@ public void observe(@NonNull SCMHead head, @NonNull SCMRevision revision) throws
_factory.decorate(project);
// ok it is now up to the observer to ensure it does the actual save.
observer.created(project);
if (isAutomaticBuild(source, head, revision, null)) {
if (isAutomaticBuild(source, head, revision, null, listener)) {
scheduleBuild(
_factory,
project,
Expand Down Expand Up @@ -2203,7 +2203,8 @@ public void observe(@NonNull SCMHead head, @NonNull SCMRevision revision) throws
private boolean isAutomaticBuild(@NonNull SCMSource source,
@NonNull SCMHead head,
@NonNull SCMRevision currRevision,
@CheckForNull SCMRevision prevRevision) {
@CheckForNull SCMRevision prevRevision,
@NonNull TaskListener listener) {
BranchSource branchSource = null;
for (BranchSource s: sources) {
if (s.getSource().getId().equals(source.getId())) {
Expand All @@ -2221,7 +2222,7 @@ private boolean isAutomaticBuild(@NonNull SCMSource source,
return !(head instanceof TagSCMHead);
} else {
for (BranchBuildStrategy s: buildStrategies) {
if (s.automaticBuild(source, head, currRevision, prevRevision)) {
if (s.automaticBuild(source, head, currRevision, prevRevision, listener)) {
return true;
}
}
Expand Down
70 changes: 66 additions & 4 deletions src/test/java/integration/EventsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.AbortException;
import hudson.Extension;
import hudson.Functions;
import hudson.model.Computer;
import hudson.model.Executor;
import hudson.model.FreeStyleProject;
import hudson.model.OneOffExecutor;
import hudson.model.Queue;
import hudson.model.Result;
import hudson.model.TaskListener;
import hudson.model.TopLevelItem;
import hudson.model.queue.QueueTaskFuture;
import integration.harness.BasicMultiBranchProject;
Expand Down Expand Up @@ -76,8 +78,10 @@
import jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy;
import jenkins.scm.api.mixin.ChangeRequestSCMHead;
import jenkins.scm.api.mixin.ChangeRequestSCMRevision;
import jenkins.scm.impl.mock.MockChangeRequestFlags;
import jenkins.scm.impl.mock.MockFailure;
import jenkins.scm.impl.mock.MockLatency;
import jenkins.scm.impl.mock.MockRepositoryFlags;
import jenkins.scm.impl.mock.MockSCMController;
import jenkins.scm.impl.mock.MockSCMDiscoverBranches;
import jenkins.scm.impl.mock.MockSCMDiscoverChangeRequests;
Expand Down Expand Up @@ -408,6 +412,40 @@ public void given_multibranchWithSourcesWantingTagsOnly_when_indexing_then_onlyT
}
}

@Test
public void
given_multibranchWithUntrustedChangeRequestBuildStrategy_when_indexing_then_onlyTrustedChangeRequestsAreFoundAndBuilt()
throws Exception {
try (MockSCMController c = MockSCMController.create()) {
c.createRepository("foo", MockRepositoryFlags.TRUST_AWARE);
c.createTag("foo", "master", "master-1.0");
Integer untrustedCrNum = c.openChangeRequest("foo", "master", MockChangeRequestFlags.UNTRUSTED);
Integer trustedCrNum = c.openChangeRequest("foo", "master");
BasicMultiBranchProject prj = r.jenkins.createProject(BasicMultiBranchProject.class, "foo");
prj.setCriteria(null);
BranchSource branchSource = new BranchSource(new MockSCMSource(c, "foo", new MockSCMDiscoverChangeRequests()));
branchSource.setBuildStrategies(Collections.singletonList(new BuildTrustedChangeRequestsStrategyImpl()));
prj.getSourcesList().add(branchSource);
prj.scheduleBuild2(0).getFuture().get();
r.waitUntilNoActivity();
assertThat("We now have projects",
prj.getItems(), not(Matchers.<FreeStyleProject>empty()));
FreeStyleProject master = prj.getItem("master");
assertThat("We have no master branch", master, nullValue());
FreeStyleProject tag = prj.getItem("master-1.0");
assertThat("We have no master-1.0 tag", tag, nullValue());
FreeStyleProject trustedCr = prj.getItem("CR-" + trustedCrNum);
FreeStyleProject untrustedCr = prj.getItem("CR-" + untrustedCrNum);
assertThat("We now have the change request", trustedCr, notNullValue());
assertThat("We have change request but no tags or branches",
prj.getItems(), containsInAnyOrder(trustedCr, untrustedCr));
r.waitUntilNoActivity();
assertThat("The trusted change request was built", trustedCr.getLastBuild(), notNullValue());
assertThat("The change request was built", trustedCr.getLastBuild().getNumber(), is(1));
assertThat("The untrusted change request was not built", untrustedCr.getLastBuild(), nullValue());
}
}

@Test
public void given_multibranchWithMergeableChangeRequests_when_indexing_then_mergableChangeRequestsBuilt()
throws Exception {
Expand Down Expand Up @@ -604,7 +642,7 @@ public static class BuildEverythingStrategyImpl extends BranchBuildStrategy {
@Override
public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head,
@NonNull SCMRevision currRevision,
SCMRevision prevRevision) {
SCMRevision prevRevision, TaskListener listener) {
return true;
}

Expand Down Expand Up @@ -654,7 +692,8 @@ public static class BuildChangeRequestsStrategyImpl extends BranchBuildStrategy
@Override
public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head,
@NonNull SCMRevision currRevision,
SCMRevision prevRevision) {
SCMRevision prevRevision,
@NonNull TaskListener listener) {
return head instanceof ChangeRequestSCMHead;
}

Expand All @@ -665,6 +704,28 @@ public static class DescriptorImpl extends BranchBuildStrategyDescriptor {
}
}

public static class BuildTrustedChangeRequestsStrategyImpl extends BranchBuildStrategy {
@Override
public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head,
@NonNull SCMRevision currRevision,
SCMRevision prevRevision, @NonNull TaskListener listener) {
if (head instanceof ChangeRequestSCMHead) {
try {
return currRevision.equals(source.getTrustedRevision(currRevision, listener));
} catch (IOException | InterruptedException e) {
Functions.printThrowable(e);
}
return true;
}
return false;
}

@TestExtension(
"given_multibranchWithUntrustedChangeRequestBuildStrategy_when_indexing_then_onlyTrustedChangeRequestsAreFoundAndBuilt")
public static class DescriptorImpl extends BranchBuildStrategyDescriptor {
}
}

@Test
public void given_multibranchWithSourcesWantingEverythingAndBuildingCRs_when_indexing_then_everythingIsFoundAndOnlyCRsBuilt()
throws Exception {
Expand Down Expand Up @@ -965,7 +1026,8 @@ public BuildRevisionStrategyImpl(String... approved) {
@Override
public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head,
@NonNull SCMRevision currRevision,
SCMRevision prevRevision) {
SCMRevision prevRevision,
@NonNull TaskListener listener) {
return currRevision instanceof MockSCMRevision
&& approved.contains(((MockSCMRevision) currRevision).getHash());
}
Expand Down Expand Up @@ -1030,7 +1092,7 @@ public IgnoreTargetChangesStrategyImpl() {
@Override
public boolean isAutomaticBuild(@NonNull SCMSource source, @NonNull SCMHead head,
@NonNull SCMRevision currRevision,
SCMRevision prevRevision) {
SCMRevision prevRevision, @NonNull TaskListener listener) {
if (currRevision instanceof ChangeRequestSCMRevision) {
ChangeRequestSCMRevision<?> currCR = (ChangeRequestSCMRevision<?>) currRevision;
if (prevRevision instanceof ChangeRequestSCMRevision) {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/jenkins/branch/BranchCategoryFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ protected TopLevelRun(@Nonnull TopLevelJob job) throws IOException {
}
}

private static abstract class MockSCMSource extends SCMSource {
public static abstract class MockSCMSource extends SCMSource {

protected MockSCMSource() {
super("1");
Expand Down

0 comments on commit e930261

Please sign in to comment.