Skip to content

Commit

Permalink
[FIXED JENKINS-44848] Stop removing JobPropertys defined outside step
Browse files Browse the repository at this point in the history 
There's some special logic for handling the case where the previous
run had the `properties` step run, but does not have the
`JobPropertyTrackerAction` on it - i.e., the first run of an existing
job with the `properties` step after upgrading. In that case, we still
use the old behavior of removing all existing `JobProperty`s. But in
all future runs, only `JobProperty`s defined via `properties` will be
removed by `properties`.

Also removed the warning messages about removing `JobProperty`s in
non-multibranch jobs, since, well, we don't do that any more once this
lands.
  • Loading branch information
@abayer
abayer committed Jun 13, 2017
1 parent 8c196ee commit c62ca44
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 67 deletions.
49 changes: 31 additions & 18 deletions src/main/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStep.java
View file
Expand Up @@ -30,7 +30,6 @@
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;
Expand All @@ -47,7 +46,10 @@
import jenkins.branch.RateLimitBranchProperty;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.jenkinsci.plugins.workflow.graphanalysis.DepthFirstScanner;
import org.jenkinsci.plugins.workflow.graphanalysis.NodeStepTypePredicate;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.steps.AbstractStepDescriptorImpl;
import org.jenkinsci.plugins.workflow.steps.AbstractStepImpl;
import org.jenkinsci.plugins.workflow.steps.AbstractSynchronousStepExecution;
Expand Down Expand Up @@ -87,7 +89,20 @@ public static class Execution extends AbstractSynchronousStepExecution<Void> {
@SuppressWarnings("unchecked") // untypable
@Override protected Void run() throws Exception {
Job<?,?> job = build.getParent();
boolean isMultibranch = isMultibranch(job);

Run<?,?> previousRun = build.getPreviousCompletedBuild();
JobPropertyTrackerAction previousAction = null;
boolean previousHadStep = false;
if (previousRun != null) {
previousAction = previousRun.getAction(JobPropertyTrackerAction.class);

// If the previous run did not have the tracker action, check to see if it ran the properties step. This
// is to deal with first run after this change is added.
if (previousRun instanceof WorkflowRun) {
previousHadStep = new DepthFirstScanner().findFirstMatch(((WorkflowRun) previousRun).getExecution(),
new NodeStepTypePredicate(step.getDescriptor())) != null;
}
}

for (JobProperty prop : step.properties) {
if (!prop.getDescriptor().isApplicable(job.getClass())) {
Expand All @@ -96,39 +111,37 @@ public static class Execution extends AbstractSynchronousStepExecution<Void> {
}
BulkChange bc = new BulkChange(job);
try {
if (!isMultibranch) {
l.getLogger().println(Messages.JobPropertyStep__could_remove_warning());
}
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(Messages.JobPropertyStep__removed_property_warning(prop.getDescriptor().getDisplayName()));
// If we have a record of JobPropertys defined via the properties step in the previous run, only
// remove those properties.
if (previousAction != null) {
if (previousAction.getJobPropertyDescriptors().contains(prop.getDescriptor().getId())) {
job.removeProperty(prop);
}
} else if (previousHadStep) {
// If the previous run did not have the tracker action but *did* run the properties step, use
// legacy behavior and remove everything.
job.removeProperty(prop);
}
job.removeProperty(prop);
}
for (JobProperty prop : step.properties) {
job.addProperty(prop);
}
bc.commit();
build.addAction(new JobPropertyTrackerAction(step.properties));
} finally {
bc.abort();
if (build.getAction(JobPropertyTrackerAction.class) == null && previousAction != null) {
build.addAction(new JobPropertyTrackerAction(previousAction));
}
}
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) {
return job.getParent() instanceof WorkflowMultiBranchProject;
}

private static final long serialVersionUID = 1L;

}
Expand Down
44 changes: 44 additions & 0 deletions src/main/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyTrackerAction.java
View file
@@ -0,0 +1,44 @@
package org.jenkinsci.plugins.workflow.multibranch;

import hudson.model.InvisibleAction;
import hudson.model.JobProperty;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

/**
* Invisible action used for tracking what {@link JobProperty}s were defined in the Jenkinsfile for a given run.
*/
public class JobPropertyTrackerAction extends InvisibleAction {
private final Set<String> jobPropertyDescriptors = new HashSet<>();

public JobPropertyTrackerAction(@CheckForNull List<JobProperty> jobProperties) {
if (jobProperties != null) {
for (JobProperty j : jobProperties) {
jobPropertyDescriptors.add(j.getDescriptor().getId());
}
}
}

/**
* Alternative constructor for copying an existing {@link JobPropertyTrackerAction}'s contents directly.
*
* @param copyFrom a non-null {@link JobPropertyTrackerAction}
*/
public JobPropertyTrackerAction(@Nonnull JobPropertyTrackerAction copyFrom) {
this.jobPropertyDescriptors.addAll(copyFrom.getJobPropertyDescriptors());
}

public Set<String> getJobPropertyDescriptors() {
return Collections.unmodifiableSet(jobPropertyDescriptors);
}

@Override
public String toString() {
return "JobPropertyTrackerAction[jobPropertyDescriptors:" + jobPropertyDescriptors + "]";
}
}
4 changes: 0 additions & 4 deletions src/main/resources/org/jenkinsci/plugins/workflow/multibranch/Messages.properties
View file
@@ -1,7 +1,3 @@
ReadTrustedStep._has_been_modified_in_an_untrusted_revis=\u2018{0}\u2019 has been modified in an untrusted revision
WorkflowMultiBranchProject.DisplayName=Multibranch Pipeline
WorkflowMultiBranchProject.Description=Creates a set of Pipeline projects according to detected branches in one SCM repository.
JobPropertyStep._could_remove_warning=WARNING: The 'properties' step will remove all 'JobProperty's currently configured in this job, \
either from the UI or from an earlier 'properties' step.\n\
This includes configuration for discarding old builds, parameters, concurrent builds and build triggers.
JobPropertyStep._removed_property_warning=WARNING: Removing existing job property ''{0}''
85 changes: 40 additions & 45 deletions src/test/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStepTest.java
View file
Expand Up @@ -230,6 +230,45 @@ public void resetStartsAndStops() {
assertNull(b3.getPreviousBuild());
}

@Issue("JENKINS-44848")
@Test public void onlyRemoveJenkinsfileProperties() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.addProperty(new DisableConcurrentBuildsJobProperty());

// Base case - not calling the properties step
p.setDefinition(new CpsFlowDefinition("echo 'Not doing anything'", true));

WorkflowRun b1 = r.buildAndAssertSuccess(p);

assertNotNull(p.getProperty(DisableConcurrentBuildsJobProperty.class));
assertNull(b1.getAction(JobPropertyTrackerAction.class));

// Adding a property, make sure the predefined one is still there.
p.setDefinition(new CpsFlowDefinition("properties([[$class: 'BuildDiscarderProperty', strategy: [$class: 'LogRotator', numToKeepStr: '1']]])", true));

WorkflowRun b2 = r.buildAndAssertSuccess(p);

assertNotNull(p.getProperty(DisableConcurrentBuildsJobProperty.class));
assertNotNull(p.getProperty(BuildDiscarderProperty.class));
JobPropertyTrackerAction action2 = b2.getAction(JobPropertyTrackerAction.class);
assertNotNull(action2);
assertEquals(1, action2.getJobPropertyDescriptors().size());
assertEquals(r.jenkins.getDescriptor(BuildDiscarderProperty.class).getId(),
action2.getJobPropertyDescriptors().iterator().next());

// Make sure the predefined property is still there after we remove the properties-step-defined property.
p.setDefinition(new CpsFlowDefinition("properties([])", true));

WorkflowRun b3 = r.buildAndAssertSuccess(p);

assertNotNull(p.getProperty(DisableConcurrentBuildsJobProperty.class));
assertNull(p.getProperty(BuildDiscarderProperty.class));

JobPropertyTrackerAction action3 = b3.getAction(JobPropertyTrackerAction.class);
assertNotNull(action3);
assertTrue(action3.getJobPropertyDescriptors().isEmpty());
}

@Issue("JENKINS-34547")
@Test public void concurrentBuildProperty() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
Expand Down Expand Up @@ -299,14 +338,6 @@ public void resetStartsAndStops() {

WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b);
// Verify that we're not seeing warnings for any properties being removed, since there are no pre-existing ones.
// Note - not using Messages here because we're not actually removing any properties.
String warningSubString = "WARNING: Removing existing job property";
assertThat(Messages.JobPropertyStep__removed_property_warning(""), containsString(warningSubString));
r.assertLogNotContains(warningSubString, b);

assertEquals(2, p.getTriggers().size());

PipelineTriggersJobProperty triggerProp = p.getTriggersJobProperty();
Expand All @@ -332,12 +363,6 @@ public void resetStartsAndStops() {

WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b2);
// Verify that we *are* seeing warnings for removing the triggers property.
String propName = r.jenkins.getDescriptorByType(PipelineTriggersJobProperty.DescriptorImpl.class).getDisplayName();
r.assertLogContains(Messages.JobPropertyStep__removed_property_warning(propName), b2);

assertNotNull(p.getTriggersJobProperty());

assertTrue(p.getTriggers().isEmpty());
Expand Down Expand Up @@ -374,14 +399,6 @@ public void resetStartsAndStops() {

WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b);
// Verify that we're not seeing warnings for any properties being removed, since there are no pre-existing ones.
// Note - not using Messages here because we're not actually removing any properties.
String warningSubString = "WARNING: Removing existing job property";
assertThat(Messages.JobPropertyStep__removed_property_warning(""), containsString(warningSubString));
r.assertLogNotContains(warningSubString, b);

assertEquals(2, p.getTriggers().size());

PipelineTriggersJobProperty triggerProp = p.getTriggersJobProperty();
Expand All @@ -408,12 +425,6 @@ public void resetStartsAndStops() {

WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b2);
// Verify that we *are* seeing warnings for removing the triggers property.
String propName = r.jenkins.getDescriptorByType(PipelineTriggersJobProperty.DescriptorImpl.class).getDisplayName();
r.assertLogContains(Messages.JobPropertyStep__removed_property_warning(propName), b2);

assertNotNull(p.getTriggersJobProperty());

assertTrue(p.getTriggers().isEmpty());
Expand All @@ -440,12 +451,6 @@ public void resetStartsAndStops() {

WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b);
// Verify that we're not seeing warnings for any properties being removed, since there are no pre-existing ones.
// Note - not using Messages here because we're not actually removing any properties.
r.assertLogNotContains("WARNING: Removing existing job property", b);

assertEquals(1, p.getTriggers().size());

PipelineTriggersJobProperty triggerProp = p.getTriggersJobProperty();
Expand All @@ -463,12 +468,6 @@ public void resetStartsAndStops() {

WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Verify that we're seeing warnings due to running 'properties' in a non-multibranch job.
r.assertLogContains(Messages.JobPropertyStep__could_remove_warning(), b2);
// Verify that we *are* seeing warnings for removing the triggers property.
String propName = r.jenkins.getDescriptorByType(PipelineTriggersJobProperty.DescriptorImpl.class).getDisplayName();
r.assertLogContains(Messages.JobPropertyStep__removed_property_warning(propName), b2);

assertNotNull(p.getTriggersJobProperty());

assertTrue(p.getTriggers().isEmpty());
Expand Down Expand Up @@ -603,7 +602,6 @@ public void noPropertiesWarnings() throws Exception {
r.waitUntilNoActivity();
WorkflowRun b1 = p.getLastBuild();
assertEquals(1, b1.getNumber());
r.assertLogNotContains(Messages.JobPropertyStep__could_remove_warning(), 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.
Expand All @@ -614,10 +612,7 @@ public void noPropertiesWarnings() throws Exception {
sampleRepo.git("add", "Jenkinsfile");
sampleRepo.git("commit", "--all", "--message=flow");

WorkflowRun b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
r.assertLogNotContains(Messages.JobPropertyStep__could_remove_warning(), b2);
String propName = r.jenkins.getDescriptorByType(DisableConcurrentBuildsJobProperty.DescriptorImpl.class).getDisplayName();
r.assertLogNotContains(Messages.JobPropertyStep__removed_property_warning(propName), b2);
r.assertBuildStatusSuccess(p.scheduleBuild2(0));
}

@Issue("JENKINS-37219")
Expand Down

0 comments on commit c62ca44

Please sign in to comment.