Skip to content

Maintain lock ordering when reporting in environment variables #301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jimklimov merged 4 commits into from
Feb 8, 2022
Merged

Maintain lock ordering when reporting in environment variables #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52d456e
maintain lock ordering when reporting in environment variables
Feb 7, 2022
90d7556
maintain lock ordering when reporting in environment variables
Feb 7, 2022
0f5dfa1
maintain lock ordering when reporting in environment variables
Feb 7, 2022
6ddeaa7
maintain lock ordering when reporting in environment variables
Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/main/java/org/jenkins/plugins/lockableresources/LockStepExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -60,7 +59,7 @@ public boolean start() throws Exception {
}

// determine if there are enough resources available to proceed
Set<LockableResource> available =
List<LockableResource> available =
LockableResourcesManager.get()
.checkResourcesAvailability(resourceHolderList, logger, null, step.skipIfLocked);
Run<?, ?> run = getContext().get(Run.class);
Expand Down Expand Up @@ -99,7 +98,7 @@ public boolean start() throws Exception {
}

public static void proceed(
final List<String> resourcenames,
final List<String> resourceNames,
StepContext context,
String resourceDescription,
final String variable,
Expand All @@ -124,7 +123,7 @@ public static void proceed(
BodyInvoker bodyInvoker =
context
.newBodyInvoker()
.withCallback(new Callback(resourcenames, resourceDescription, inversePrecedence));
.withCallback(new Callback(resourceNames, resourceDescription, inversePrecedence));
if (variable != null && variable.length() > 0) {
// set the variable for the duration of the block
bodyInvoker.withContext(
Expand All @@ -136,10 +135,10 @@ public static void proceed(
@Override
public void expand(@NonNull EnvVars env) {
final Map<String, String> variables = new HashMap<>();
final String resources = String.join(",", resourcenames);
final String resources = String.join(",", resourceNames);
variables.put(variable, resources);
for (int index = 0; index < resourcenames.size(); ++index) {
variables.put(variable + index, resourcenames.get(index));
for (int index = 0; index < resourceNames.size(); ++index) {
variables.put(variable + index, resourceNames.get(index));
}
LOGGER.finest("Setting "
+ variables.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining(", "))
Expand Down
21 changes: 11 additions & 10 deletions src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -405,13 +406,13 @@ private boolean checkCurrentResourcesStatus(
}

public synchronized boolean lock(
Set<LockableResource> resources, Run<?, ?> build, @Nullable StepContext context) {
List<LockableResource> resources, Run<?, ?> build, @Nullable StepContext context) {
Copy link
Copy Markdown

@joshsleeper joshsleeper May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I'm by no means a java person, but I am a dev who uses Jenkins.
can someone explain to me why this param type change from Set to List isn't considered a breaking change, or even worth a callout in the release notes?

it breaks groovy jobs like mine that call lock() directly because it was previously being created as a Set to match the API.

is this a release notes/documentation failure, or am I misusing the plugin maybe?

Copy link
Copy Markdown
Contributor Author

@gaspardpetit gaspardpetit May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @joshsleeper

Definitely a failure to report in the release notes. While I am an experienced Java developer, I only just recently started contributing to Jenkins plugins. My experience with them is to only use operations exposed as steps, and I initially did not realize some people were calling internal plug-in methods directly and had backward compatibility expectations on them. I'll definitely be more careful to flag these breaking changes going forward and for now I can only apologize.

Gaspard

Copy link
Copy Markdown

@joshsleeper joshsleeper May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no anger here and plenty of understanding~
this isn't the first upgrade of this plugin that's busted things for us, so figured we were better off calling it out in the hopes that it doesn't happen every time!

I would suggest, if possible, to consider updating the release notes to mention the breaking change~
after all, changing the calling pattern of anything that's public is a breaking change! 😆

that aside, jenkins plugin maintenance def isn't a job folks do for glory so thanks for all your hard work!

return lock(resources, build, context, null, null, false);
}

/** Try to lock the resource and return true if locked. */
public synchronized boolean lock(
Set<LockableResource> resources,
List<LockableResource> resources,
Run<?, ?> build,
@Nullable StepContext context,
@Nullable String logmessage,
Expand Down Expand Up @@ -517,7 +518,7 @@ public synchronized void unlockNames(
return;
}

Set<LockableResource> requiredResourceForNextContext =
List<LockableResource> requiredResourceForNextContext =
checkResourcesAvailability(
nextContext.getResources(), null, remainingResourceNamesToUnLock);

Expand Down Expand Up @@ -798,7 +799,7 @@ public synchronized void unreserve(List<LockableResource> resources) {
}

// remove context from queue and process it
Set<LockableResource> requiredResourceForNextContext =
List<LockableResource> requiredResourceForNextContext =
checkResourcesAvailability(
nextContext.getResources(), nextContextLogger,
null, resourceNamesToUnreserve);
Expand Down Expand Up @@ -917,7 +918,7 @@ public boolean configure(StaplerRequest req, JSONObject json) {
}

/** @see #checkResourcesAvailability(List, PrintStream, List, List, boolean) */
public synchronized Set<LockableResource> checkResourcesAvailability(
public synchronized List<LockableResource> checkResourcesAvailability(
List<LockableResourcesStruct> requiredResourcesList,
@Nullable PrintStream logger,
@Nullable List<String> lockedResourcesAboutToBeUnlocked) {
Expand All @@ -927,7 +928,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(
}

/** @see #checkResourcesAvailability(List, PrintStream, List, List, boolean) */
public synchronized Set<LockableResource> checkResourcesAvailability(
public synchronized List<LockableResource> checkResourcesAvailability(
List<LockableResourcesStruct> requiredResourcesList,
@Nullable PrintStream logger,
@Nullable List<String> lockedResourcesAboutToBeUnlocked,
Expand All @@ -937,7 +938,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(
}

/** @see #checkResourcesAvailability(List, PrintStream, List, List, boolean) */
public synchronized Set<LockableResource> checkResourcesAvailability(
public synchronized List<LockableResource> checkResourcesAvailability(
List<LockableResourcesStruct> requiredResourcesList,
@Nullable PrintStream logger,
@Nullable List<String> lockedResourcesAboutToBeUnlocked,
Expand All @@ -956,7 +957,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(
* requiredResources and returns the necessary available resources. If not enough resources are
* available, returns null.
*/
public synchronized Set<LockableResource> checkResourcesAvailability(
public synchronized List<LockableResource> checkResourcesAvailability(
List<LockableResourcesStruct> requiredResourcesList,
@Nullable PrintStream logger,
@Nullable List<String> lockedResourcesAboutToBeUnlocked,
Expand Down Expand Up @@ -1082,7 +1083,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(
}

// Find remaining resources
Set<LockableResource> allSelected = new HashSet<>();
LinkedHashSet<LockableResource> allSelected = new LinkedHashSet<>();

for (LockableResourcesCandidatesStruct requiredResources : requiredResourcesCandidatesList) {
List<LockableResource> candidates = requiredResources.candidates;
Expand Down Expand Up @@ -1131,7 +1132,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(
allSelected.addAll(selected);
}

return allSelected;
return new ArrayList<>(allSelected);
}

/*
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@
import hudson.model.TaskListener;
import hudson.model.listeners.RunListener;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import org.jenkins.plugins.lockableresources.LockableResource;
Expand All @@ -45,7 +43,7 @@ public void onStarted(Run<?, ?> build, TaskListener listener) {

if (build instanceof AbstractBuild) {
Job<?, ?> proj = Utils.getProject(build);
Set<LockableResource> required = new HashSet<>();
List<LockableResource> required = new ArrayList<>();
if (proj != null) {
LockableResourcesStruct resources = Utils.requiredResources(proj);

Expand All @@ -57,6 +55,9 @@ public void onStarted(Run<?, ?> build, TaskListener listener) {
required.addAll(resources.required);
}

// make sure each entry is unique
required = new ArrayList<>(new LinkedHashSet<>(required));

if (LockableResourcesManager.get().lock(required, build, null)) {
build.addAction(LockedResourcesBuildAction
.fromResources(required));
Expand Down
44 changes: 44 additions & 0 deletions src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.util.Collections;
import java.util.List;
import java.util.concurrent.CyclicBarrier;
import java.util.stream.Collectors;
import net.sf.json.JSONObject;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
Expand Down Expand Up @@ -848,6 +849,49 @@ public void multipleLocksFillVariables() throws Exception {
j.assertLogContains("VAR2 IS null", b1);
}


@Test
//@Issue("JENKINS-XXXXX")
public void locksInVariablesAreInTheRequestedOrder() throws Exception {
List<String> extras = new ArrayList<>();
List<String> eachVar = new ArrayList<>();
for (int i = 0; i < 100; ++i) {
LockableResourcesManager.get().createResource("extra" + i);
extras.add("[resource: 'extra" + i + "', quantity:1]");
eachVar.add(" echo \"VAR" + (i+1) + " IS ${env.var" + (i+1) + "}\"\n");
}
LockableResourcesManager.get().createResource("main");

WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(
new CpsFlowDefinition(
"lock(variable: 'var', resource: 'main', quantity: 1, extra: ["
+ extras.stream().collect(Collectors.joining(","))
+ "]) {\n"
+ " echo \"VAR IS ${env.var}\"\n"
+ " echo \"VAR0 IS ${env.var0}\"\n"
+ eachVar.stream().collect(Collectors.joining("\n"))
+ "}",
true));
WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
j.waitForCompletion(b1);

// Variable should have been filled
j.assertLogContains("VAR IS main,extra0,extra1,extra2,extra3,extra4,extra5,extra6,extra7,extra8,extra9,extra10,extra11,"
+ "extra12,extra13,extra14,extra15,extra16,extra17,extra18,extra19,extra20,extra21,extra22,extra23,extra24,extra25,extra26,"
+ "extra27,extra28,extra29,extra30,extra31,extra32,extra33,extra34,extra35,extra36,extra37,extra38,extra39,extra40,extra41,"
+ "extra42,extra43,extra44,extra45,extra46,extra47,extra48,extra49,extra50,extra51,extra52,extra53,extra54,extra55,extra56,"
+ "extra57,extra58,extra59,extra60,extra61,extra62,extra63,extra64,extra65,extra66,extra67,extra68,extra69,extra70,extra71,"
+ "extra72,extra73,extra74,extra75,extra76,extra77,extra78,extra79,extra80,extra81,extra82,extra83,extra84,extra85,extra86,"
+ "extra87,extra88,extra89,extra90,extra91,extra92,extra93,extra94,extra95,extra96,extra97,extra98,extra99", b1);

j.assertLogContains("VAR0 IS main", b1);
for (int i = 0; i < 100; ++i) {
j.assertLogContains("VAR" + (i+1) + " IS extra" + i, b1);
}

}

@Test
@Issue("JENKINS-50176")
public void lockWithLabelFillsVariable() throws Exception {
Expand Down