[FIXED JENKINS-37005] Warn users using properties outside multibranch

This doesn't change actual behavior. It simply logs to the console when the properties step is invoked outside of a multibranch job, including listing any properties that are removed. The goal is to decrease confusion if/when a non-multibranch job suddenly loses its triggers, etc due to a properties step being executed in the job.
jenkinsci · Jul 28, 2016 · 232eb75 · 232eb75
1 parent 0342ade
commit 232eb75
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 7 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStep.java b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStep.java
@@ -30,6 +30,7 @@
 import hudson.ExtensionList;
 import hudson.model.Descriptor;
 import hudson.model.DescriptorVisibilityFilter;
+import hudson.model.ItemGroup;
 import hudson.model.Job;
 import hudson.model.JobProperty;
 import hudson.model.JobPropertyDescriptor;
@@ -40,6 +41,8 @@
 import java.util.Map;
 import java.util.Set;
 import javax.inject.Inject;
+
+import hudson.model.TaskListener;
 import jenkins.branch.BuildRetentionBranchProperty;
 import jenkins.branch.RateLimitBranchProperty;
 import jenkins.model.Jenkins;
@@ -82,19 +85,31 @@ public static class Execution extends AbstractSynchronousStepExecution<Void> {
 
         @SuppressWarnings("unchecked") // untypable
         @Override protected Void run() throws Exception {
+            TaskListener l = getContext().get(TaskListener.class);
             Job<?,?> job = build.getParent();
+            boolean isMultibranch = isMultibranch(job);
+
             for (JobProperty prop : step.properties) {
                 if (!prop.getDescriptor().isApplicable(job.getClass())) {
                     throw new AbortException("cannot apply " + prop.getDescriptor().getId() + " to a " + job.getClass().getSimpleName());
                 }
             }
             BulkChange bc = new BulkChange(job);
             try {
+                if (!isMultibranch) {
+                    l.getLogger().println("WARNING: The 'properties' step will remove all 'JobProperty's currently "
+                            + "configured in this job, either from the UI or from an earlier 'properties' step.");
+                    l.getLogger().println("This includes configuration for discarding old builds, parameters, "
+                            + "concurrent builds and build triggers.");
+                }
                 for (JobProperty prop : job.getAllProperties()) {
                     if (prop instanceof BranchJobProperty) {
                         // TODO do we need to define an API for other properties which should not be removed?
                         continue;
                     }
+                    if (!isMultibranch) {
+                        l.getLogger().println("WARNING: Removing existing job property '" + prop.getDescriptor().getDisplayName() + "'");
+                    }
                     job.removeProperty(prop);
                 }
                 for (JobProperty prop : step.properties) {
@@ -107,6 +122,20 @@ public static class Execution extends AbstractSynchronousStepExecution<Void> {
             return null;
         }
 
+        /**
+         * Returns true if we're in a multibranch job - behavior may be slightly different when that's not the case.
+         *
+         * @param job The job this build belongs to.
+         * @return True if this is a multibranch job, false otherwise.
+         */
+        private boolean isMultibranch(Job<?,?> job) {
+            ItemGroup<?> parent = job.getParent();
+            if (!(parent instanceof WorkflowMultiBranchProject)) {
+                return false;
+            }
+            return true;
+        }
+
         private static final long serialVersionUID = 1L;
 
     }

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStepTest.java
@@ -25,7 +25,6 @@
 package org.jenkinsci.plugins.workflow.multibranch;
 
 import hudson.model.BooleanParameterDefinition;
-import hudson.model.Item;
 import hudson.model.JobProperty;
 import hudson.model.ParametersAction;
 import hudson.model.ParametersDefinitionProperty;
@@ -34,21 +33,22 @@
 import hudson.model.queue.QueueTaskFuture;
 import hudson.tasks.LogRotator;
 
-import java.io.ObjectStreamException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
 import hudson.triggers.TimerTrigger;
 import hudson.triggers.Trigger;
-import hudson.triggers.TriggerDescriptor;
 import jenkins.branch.BranchProperty;
 import jenkins.branch.BranchSource;
+import jenkins.branch.DefaultBranchPropertyStrategy;
 import jenkins.model.BuildDiscarder;
 import jenkins.model.BuildDiscarderProperty;
 import jenkins.plugins.git.GitSCMSource;
 import jenkins.plugins.git.GitSampleRepoRule;
 import org.jenkinsci.Symbol;
+import jenkins.scm.api.SCMHead;
+import jenkins.scm.api.SCMSource;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
@@ -65,8 +65,6 @@
 import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
-import org.jvnet.hudson.test.TestExtension;
-import org.kohsuke.stapler.DataBoundConstructor;
 
 import static org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject;
 
@@ -255,7 +253,12 @@ public class JobPropertyStepTest {
                                 + "    [$class: 'MockTrigger']])])\n"
                 ) + "echo 'foo'"));
 
-        r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+
+        // Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
+        r.assertLogContains("WARNING: The 'properties' step will remove all 'JobProperty's currently configured in this job, either from the UI or from an earlier 'properties' step.", b);
+        // Verify that we're not seeing warnings for any properties being removed, since there are no pre-existing ones.
+        r.assertLogNotContains("WARNING: Removing existing job property", b);
 
         assertEquals(2, p.getTriggers().size());
 
@@ -280,7 +283,12 @@ public class JobPropertyStepTest {
         p.setDefinition(new CpsFlowDefinition("properties([disableConcurrentBuilds()])\n"
                 + "echo 'foo'"));
 
-        r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+
+        // Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
+        r.assertLogContains("WARNING: The 'properties' step will remove all 'JobProperty's currently configured in this job, either from the UI or from an earlier 'properties' step.", b2);
+        // Verify that we *are* seeing warnings for removing the triggers property.
+        r.assertLogContains("WARNING: Removing existing job property 'Build triggers'", b2);
 
         assertNotNull(p.getTriggersJobProperty());
 
@@ -289,6 +297,40 @@ public class JobPropertyStepTest {
         assertEquals("[null, false, null]", MockTrigger.startsAndStops.toString());
     }
 
+    @Issue("JENKINS-37005")
+    @Test 
+    public void noPropertiesWarnings() throws Exception {
+        sampleRepo.init();
+        sampleRepo.write("Jenkinsfile", "echo \"branch=${env.BRANCH_NAME}\"\n"
+                + "properties([[$class: 'DisableConcurrentBuildsJobProperty']])");
+        sampleRepo.write("file", "initial content");
+        sampleRepo.git("add", "Jenkinsfile");
+        sampleRepo.git("commit", "--all", "--message=flow");
+        WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
+        mp.getSourcesList().add(new BranchSource(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", false), new DefaultBranchPropertyStrategy(new BranchProperty[0])));
+        for (SCMSource source : mp.getSCMSources()) {
+            assertEquals(mp, source.getOwner());
+        }
+        WorkflowJob p = scheduleAndFindBranchProject(mp, "master");
+        assertEquals(new SCMHead("master"), SCMHead.HeadByItem.findHead(p));
+        assertEquals(1, mp.getItems().size());
+        r.waitUntilNoActivity();
+        WorkflowRun b1 = p.getLastBuild();
+        assertEquals(1, b1.getNumber());
+        r.assertLogNotContains("WARNING: The 'properties' step will remove all 'JobProperty's currently configured in this job, either from the UI or from an earlier 'properties' step.", b1);
+
+        // Now verify that we don't get any messages about removing properties when a property actually gets removed as
+        // we add a new one.
+        sampleRepo.write("Jenkinsfile", "echo \"branch=${env.BRANCH_NAME}\"\n"
+                + "properties([[$class: 'BuildDiscarderProperty', strategy: [$class: 'LogRotator', numToKeepStr: '1']]])");
+        sampleRepo.git("add", "Jenkinsfile");
+        sampleRepo.git("commit", "--all", "--message=flow");
+
+        WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        r.assertLogNotContains("WARNING: The 'properties' step will remove all 'JobProperty's currently configured in this job, either from the UI or from an earlier 'properties' step.", b2);
+        r.assertLogNotContains("WARNING: Removing existing job property", b2);
+    }
+
     private <T extends Trigger> T getTriggerFromList(Class<T> clazz, List<Trigger<?>> triggers) {
         for (Trigger t : triggers) {
             if (clazz.isInstance(t)) {