Skip to content
Permalink
Browse files

[FIXED JENKINS-44848] Stop removing JobPropertys defined outside step

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 committed Jun 13, 2017
1 parent 8c196ee commit c62ca446066454e5834c7e641a3fff157e8184e1
@@ -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;
@@ -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;
@@ -87,7 +89,20 @@
@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())) {
@@ -96,39 +111,37 @@
}
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;

}
@@ -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 + "]";
}
}
@@ -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}''
@@ -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");
@@ -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();
@@ -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());
@@ -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();
@@ -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());
@@ -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();
@@ -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());
@@ -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.
@@ -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")

0 comments on commit c62ca44

Please sign in to comment.
You can’t perform that action at this time.