Re-allow null / empty values for step params and always trim label and resource name #788

eva-mueller-coremedia · 2025-06-15T09:40:02Z

First of all, thank you for this great plugin.

This PR mainly aims to solve #615 and #619 by re-allowing empty resp. null values for the lock step.

A breaking change has been introduced with #579 stateing

This makes no sense and cost only performance. It is definitely user (programer) failure.

and #619 highlights the if condition which has been introduced with #579.

@mPokornyETM responds to this in #619 (comment) with

When you remove the if statement, you will run into many side effects. You can trust me.
Anyway, I think it is user failure to call the function lock() with null or empty string.

See #619

Preamble

Feel free to reject this PR if you want to stick with failing on empty resp. null values for the lock step.
I see the point to treat empty resp. null values as error but also understand that such a breaking change in a huge ecosystem of Jenkins pipelines is hard to handle. So PR can be some kind of smooth transition.

If you reject & close this PR, it would be great if you could also close #619 and #615 with some kind of explanation. Thank you.

What has been done

Re-allow empty resp. null values for the lock step (9b234ec)
Trim spaces from label and resource name (6825db7)
Some housekeeping
Rename test classes so that maven-surefire-plugin v3.5.3 executes them (https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#includes)

This PR solves

Empty ressource is not allowed anymore

Fixes #619

Use of [] for extra to not lock is not working anymore with 1232.v512d6c434eb_d release

Fixes #615

Trim spaces from resource name, otherwise it will stuck

Fixes #612

Testing done

I added an additional test class LockStepNothingTest.java to verify that the changes work and extended some tests to ensure that trimming (#612) works as well.

Submitter checklist

The Jira / Github issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
- The changelog generator for plugins uses the pull request title as the changelog entry.
- Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during the upgrade.
There is automated testing

Maintainer checklist

Before the changes are marked as ready-for-merge:

There is at least one (1) approval for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically. See also release-drafter-labels.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
java code changes are tested by automated test.
…thing.

mPokornyETM · 2025-06-16T08:33:07Z

@eva-mueller-coremedia thx for contributing. Basically I am fine with the changes. Please check my comments. One more hint. Maybe we can allow this scenario with some property like 'allowEmpty' : true.

eva-mueller-coremedia · 2025-06-16T08:38:35Z

@eva-mueller-coremedia thx for contributing. Basically I am fine with the changes. Please check my comments. One more hint. Maybe we can allow this scenario with some property like 'allowEmpty' : true.

Hi @mPokornyETM - sure, I can try to add this config property.

Please check my comments.

Did you submit your review? Unfortunately, I do not see any comments

eva-mueller-coremedia · 2025-06-17T06:43:17Z

@mPokornyETM

Newly changes/improvements

Add configuration option to allow empty or null values - 565fd69
Improve trim spaces from label and resource name - 72a55ad

Unfortunately, I still cannot see your comments

eva-mueller-coremedia · 2025-06-23T09:44:17Z

Hi @mPokornyETM - please consider this comment as friendly nudge. Would be great to get a review. Are you still on your own on this project?

mPokornyETM

LGTM

src/main/java/org/jenkins/plugins/lockableresources/LockStep.java

mPokornyETM · 2025-06-16T08:10:45Z

src/main/java/org/jenkins/plugins/lockableresources/LockStepExecution.java

@@ -55,62 +55,74 @@ public boolean start() throws Exception {
        List<LockableResource> available = null;
        LinkedHashMap<String, List<LockableResourceProperty>> lockedResources = new LinkedHashMap<>();
        LockableResourcesManager lrm = LockableResourcesManager.get();
-        synchronized (lrm.syncResources) {
+        synchronized (LockableResourcesManager.syncResources) {


Why you has changed to static call? I amn wondering, that this is not syntax failure,

I learned that accessing static members in Java by instance reference is considered as bad practice since static fields and methods belong to the class, not an instance. I.e., static fields or methods should be accessed via the class name.

src/main/java/org/jenkins/plugins/lockableresources/LockStepExecution.java

src/main/java/org/jenkins/plugins/lockableresources/LockStepResource.java

mPokornyETM · 2025-06-16T08:27:36Z

src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

@@ -57,18 +57,18 @@
 public class LockableResourcesManager extends GlobalConfiguration {

    /** Object to synchronized operations over LRM */
-    public static final transient Object syncResources = new Object();
+    public static final Object syncResources = new Object();


why you change it to static?

I did not change this to static, I removed the transient modifier since the transient modifier is redundant for a static field. Only instance fields are serialized.

mPokornyETM · 2025-06-16T08:28:00Z

src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

@@ -364,7 +363,7 @@ private String getStack() {
    /** Checks if given resource exist. */
    @NonNull
    @Restricted(NoExternalUse.class)
-    public boolean resourceExist(@CheckForNull String resourceName) {
+    public Boolean resourceExist(@CheckForNull String resourceName) {


Because primitive type members like boolean cannot be annotated with - for example - @NonNull.

src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

…thing. Fixes jenkinsci#615 Fixes jenkinsci#619

Fixes jenkinsci#612

regex '[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*)IT(Case)?'

to provide a hint in case of an endless loop

since class 'LockedResourcesBuildAction' is not serializable

instead of build parameter since there is no control over the identity, visibility, or lifecycle of that object.

with the same value in case of a NPE.

Fixes jenkinsci#612

eva-mueller-coremedia marked this pull request as ready for review June 15, 2025 12:48

eva-mueller-coremedia requested a review from a team as a code owner June 15, 2025 12:48

eva-mueller-coremedia marked this pull request as draft June 16, 2025 22:43

eva-mueller-coremedia force-pushed the re-allow-nothing branch 2 times, most recently from dd2e7e2 to 171c2a2 Compare June 17, 2025 06:16

eva-mueller-coremedia marked this pull request as ready for review June 17, 2025 06:40

eva-mueller-coremedia force-pushed the re-allow-nothing branch 2 times, most recently from 6e853d1 to d6ba2fa Compare June 24, 2025 14:57

mPokornyETM approved these changes Jul 7, 2025

View reviewed changes

mPokornyETM added bug java Pull requests that update Java code ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.) documentation test localization labels Jul 7, 2025

eva-mueller-coremedia added 11 commits July 7, 2025 22:15

Treat null or empty for label, resource, or extra as noop and lock no…

386f5d2

…thing. Fixes jenkinsci#615 Fixes jenkinsci#619

chore: fix typos

f817771

Trim spaces from label and resource name

d87ba99

Fixes jenkinsci#612

chore: Remove comments

86e8262

chore: Rename Tests to match

dd89a35

regex '[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*)IT(Case)?'

chore: Argument 'run.getExecution()' might be null

24dafc8

chore: simplify statements

b099c1a

chore: extract code to own method from lengthy method

1aafc0e

chore: remove unused code

5283e0e

chore: Primitive type members cannot be annotated

9599565

chore: Modifier 'transient' is redundant for a 'static' field

7fef819

eva-mueller-coremedia added 15 commits July 7, 2025 22:15

Add additional log statement

1d1f383

to provide a hint in case of an endless loop

chore: Use final modifier

3a3a72b

chore: Remove serialVersionUID

87e1902

since class 'LockedResourcesBuildAction' is not serializable

Synchronize on build ID

7830535

instead of build parameter since there is no control over the identity, visibility, or lifecycle of that object.

chore: do not access static members via instance reference

3eaf324

chore: do not re-assign variable

4a94aea

with the same value in case of a NPE.

chore: fix log statement

c359f6d

Fix test LockStepManualUnreserveUnblocksJobTest

e05a0a5

Format test

7236916

Add configuration option to allow empty or null values

152caf9

Trim spaces from label and resource name

2027aea

Fixes jenkinsci#612

chore: Remove redundant initializer

d2f74df

Add documentation

20bb8f0

Log warning if provided parameter starts or ends with spaces.

796b25f

Use Util.fixEmptyAndTrim to handle input

5c9ade3

eva-mueller-coremedia force-pushed the re-allow-nothing branch from d6ba2fa to 85120a3 Compare July 7, 2025 21:43

Add more tests to test trimmed parameter

3ecfe27

eva-mueller-coremedia force-pushed the re-allow-nothing branch from 85120a3 to 3ecfe27 Compare July 7, 2025 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Re-allow null / empty values for step params and always trim label and resource name #788

Re-allow null / empty values for step params and always trim label and resource name #788

Uh oh!

eva-mueller-coremedia commented Jun 15, 2025 •

edited

Loading

Uh oh!

mPokornyETM commented Jun 16, 2025

Uh oh!

eva-mueller-coremedia commented Jun 16, 2025 •

edited

Loading

Uh oh!

eva-mueller-coremedia commented Jun 17, 2025

Uh oh!

eva-mueller-coremedia commented Jun 23, 2025

Uh oh!

mPokornyETM left a comment

Uh oh!

Uh oh!

Uh oh!

mPokornyETM Jun 16, 2025

Uh oh!

eva-mueller-coremedia Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mPokornyETM Jun 16, 2025

Uh oh!

eva-mueller-coremedia Jul 7, 2025 •

edited

Loading

Uh oh!

mPokornyETM Jun 16, 2025

Uh oh!

eva-mueller-coremedia Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Re-allow null / empty values for step params and always trim label and resource name #788

Are you sure you want to change the base?

Re-allow null / empty values for step params and always trim label and resource name #788

Uh oh!

Conversation

eva-mueller-coremedia commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preamble

What has been done

This PR solves

Empty ressource is not allowed anymore

Use of [] for extra to not lock is not working anymore with 1232.v512d6c434eb_d release

Trim spaces from resource name, otherwise it will stuck

Testing done

Submitter checklist

Maintainer checklist

Uh oh!

mPokornyETM commented Jun 16, 2025

Uh oh!

eva-mueller-coremedia commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eva-mueller-coremedia commented Jun 17, 2025

Uh oh!

eva-mueller-coremedia commented Jun 23, 2025

Uh oh!

mPokornyETM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mPokornyETM Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

eva-mueller-coremedia Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mPokornyETM Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

eva-mueller-coremedia Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mPokornyETM Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

eva-mueller-coremedia Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eva-mueller-coremedia commented Jun 15, 2025 •

edited

Loading

eva-mueller-coremedia commented Jun 16, 2025 •

edited

Loading

eva-mueller-coremedia Jul 7, 2025 •

edited

Loading