Fix JENKINS-48826 by using DurabilityHint provider with the BranchPro…

…perty dynamically
jenkinsci · Jan 9, 2018 · 2694ac5 · 2694ac5
1 parent 26636c7
commit 2694ac5
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 31 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/DurabilityHintBranchProperty.java b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/DurabilityHintBranchProperty.java
@@ -1,28 +1,34 @@
 package org.jenkinsci.plugins.workflow.multibranch;
 
-import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Extension;
+import hudson.model.Item;
 import hudson.model.Job;
-import hudson.model.JobProperty;
 import hudson.model.Run;
 import jenkins.branch.BranchProperty;
 import jenkins.branch.BranchPropertyDescriptor;
+import jenkins.branch.BranchPropertyStrategy;
 import jenkins.branch.JobDecorator;
+import jenkins.branch.MultiBranchProject;
+import jenkins.scm.api.SCMSource;
+import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider;
 import org.jenkinsci.plugins.workflow.flow.FlowDurabilityHint;
 import org.jenkinsci.plugins.workflow.flow.GlobalDefaultFlowDurabilityLevel;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
-import org.jenkinsci.plugins.workflow.job.properties.DurabilityHintJobProperty;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
-import java.util.Iterator;
-import java.util.List;
+import java.util.Optional;
 
 /**
  * Branch property so we can define per-branch durability policies, i.e. so feature branches aren't built durably but master is.
  * Also lets us set the durability level before the pipeline has run (a step ahead of the "properties" step).
+ *
+ * This implementation is designed so that each build will freshly evaluate the {@link FlowDurabilityHint} provided by {@link BranchPropertyStrategy}
+ *  thus sidestepping issues with failing to update along with the BranchPropertyStrategy (JENKINS-48826).
+ *
  * @author Sam Van Oort
  */
 @Restricted(NoExternalUse.class)
@@ -39,36 +45,17 @@ public DurabilityHintBranchProperty(@Nonnull FlowDurabilityHint hint) {
         this.hint = hint;
     }
 
+    /** No-op impl because we only care about the actual BranchProperty attached. */
     @Override
     public final <P extends Job<P, B>, B extends Run<P, B>> JobDecorator<P, B> jobDecorator(Class<P> clazz) {
-        if (!clazz.isAssignableFrom(WorkflowJob.class)) {
-            return null;
-        }
-        return new JobDecorator<P, B>() {
-            @NonNull
-            @Override
-            public List<JobProperty<? super P>> jobProperties(
-                    @NonNull List<JobProperty<? super P>> jobProperties) {
-                List<JobProperty<? super P>> result = asArrayList(jobProperties);
-                for (Iterator<JobProperty<? super P>> iterator = result.iterator(); iterator.hasNext(); ) {
-                    JobProperty<? super P> p = iterator.next();
-                    if (p instanceof DurabilityHintJobProperty) {
-                        iterator.remove();
-                    }
-                }
-                if (getHint() != null) {
-                    result.add((JobProperty)(new DurabilityHintJobProperty(getHint())));
-                }
-                return result;
-            }
-        };
+        return null;
     }
 
     @Extension
-    public static class DescriptorImpl extends BranchPropertyDescriptor {
+    public static class DescriptorImpl extends BranchPropertyDescriptor implements DurabilityHintProvider {
         @Override
         public String getDisplayName() {
-            return "Pipeline speed/durability override";
+            return "Pipeline branch speed/durability override";
         }
 
         public FlowDurabilityHint[] getDurabilityHintValues() {
@@ -78,5 +65,43 @@ public FlowDurabilityHint[] getDurabilityHintValues() {
         public static FlowDurabilityHint getDefaultDurabilityHint() {
             return GlobalDefaultFlowDurabilityLevel.getDefaultDurabilityHint();
         }
+
+        @Override
+        /** Lower ordinal than {@link org.jenkinsci.plugins.workflow.job.properties.DurabilityHintJobProperty} so those can override. */
+        public int ordinal() {
+            return 200;
+        }
+
+        @CheckForNull
+        @Override
+        /**
+         * Dynamically fetch the property with each build, because the {@link BranchPropertyStrategy} does not re-evaluate,
+         * resulting in {@see <a href="https://issues.jenkins-ci.org/browse/JENKINS-48826">JENKINS-48826</a>}. */
+        public FlowDurabilityHint suggestFor(@Nonnull Item x) {
+            // BranchJobProperty *should* be present if it's a child of a MultiBranchProject but we double-check for safety
+            if (x instanceof WorkflowJob && x.getParent() instanceof MultiBranchProject && ((WorkflowJob)x).getProperty(BranchJobProperty.class) != null) {
+                MultiBranchProject mp  = (MultiBranchProject)(x.getParent());
+                WorkflowJob job = (WorkflowJob)x;
+                BranchJobProperty bjp = job.getProperty(BranchJobProperty.class);
+
+                String sourceId = bjp.getBranch().getSourceId();
+                if (sourceId != null) {
+                    SCMSource source = mp.getSCMSource(sourceId);
+                    if (source != null) {
+                        BranchPropertyStrategy bps = mp.getBranchPropertyStrategy(source);
+                        if (bps != null) {
+                            Optional<BranchProperty> props = bps.getPropertiesFor(bjp.getBranch().getHead()).stream().filter(
+                                    bp -> bp instanceof DurabilityHintBranchProperty
+                            ).findFirst();
+                            if (props.isPresent()) {
+                                return ((DurabilityHintBranchProperty)props.get()).getHint();
+                            }
+                        }
+                    }
+                }
+
+            }
+            return null;
+        }
     }
 }
diff --git a/.../org/jenkinsci/plugins/workflow/multibranch/DurabilityHintBranchPropertyWorkflowTest.java b/.../org/jenkinsci/plugins/workflow/multibranch/DurabilityHintBranchPropertyWorkflowTest.java
@@ -32,11 +32,14 @@
 import jenkins.branch.NoTriggerOrganizationFolderProperty;
 import jenkins.plugins.git.GitSCMSource;
 import jenkins.plugins.git.GitSampleRepoRule;
+import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider;
 import org.jenkinsci.plugins.workflow.flow.FlowDurabilityHint;
+import org.jenkinsci.plugins.workflow.flow.GlobalDefaultFlowDurabilityLevel;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.job.properties.DurabilityHintJobProperty;
 import org.junit.Assert;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -94,7 +97,9 @@ public void configRoundtrip() throws Exception {
         Assert.assertEquals(FlowDurabilityHint.SURVIVABLE_NONATOMIC, prop.getHint());
     }
 
-    @Test public void durabilityHintByBranchProperty() throws Exception {
+    @Test
+    @Issue("JENKINS-48826")
+    public void durabilityHintByBranchProperty() throws Exception {
         sampleRepo.init();
         sampleRepo.write("Jenkinsfile",
                         "echo 'whynot'");
@@ -109,9 +114,15 @@ public void configRoundtrip() throws Exception {
         WorkflowJob p = scheduleAndFindBranchProject(mp, "master");
         r.waitUntilNoActivity();
 
-        Assert.assertEquals(FlowDurabilityHint.SURVIVABLE_NONATOMIC, p.getProperty(DurabilityHintJobProperty.class).getHint());
+        Assert.assertEquals(FlowDurabilityHint.SURVIVABLE_NONATOMIC, DurabilityHintProvider.suggestedFor(p));
         WorkflowRun b1 = p.getLastBuild();
         Assert.assertEquals(Result.SUCCESS, b1.getResult());
-    }
 
+        // Ensure when we remove the property, branches see that on the next build
+        bs.setStrategy(new DefaultBranchPropertyStrategy(new BranchProperty[]{}));
+        p = scheduleAndFindBranchProject(mp, "master");
+        r.waitUntilNoActivity();
+
+        Assert.assertEquals(GlobalDefaultFlowDurabilityLevel.getDefaultDurabilityHint(), DurabilityHintProvider.suggestedFor(mp.getItems().iterator().next()));
+    }
 }