Converting some Groovy sources to Java #765

jglick · 2025-02-25T20:23:08Z

Experimenting with whether Copilot is able to figure out how to do this efficiently. Only considering src/main/groovy/; src/main/resources/**/*.groovy are CPS-transformed.

…ignable

jglick · 2025-02-25T21:47:48Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

This file is right at the similarity limit for Git, so any further edits would risk making the diff unreviewable!

…which is causing a problem since resolving SpotBugs violations puts it over the limit.

🤔 maybe I could merge a change to trunk first which just pads out the *.groovy file with lots of long multiline comments that say nothing, and then leave them there, so that Git considers them unmodified.

(Mercurial IMO is better in this regard since hg mv explicitly records the rename, rather than leaving subsequent diff commands to guess at whether it might have happened.)

jglick · 2025-02-25T21:48:21Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-    @Restricted(NoExternalUse.class)
-    static Map<String, CredentialWrapper> getLegacyEnvCredentials(@NonNull Environment environment) {
-        Map<String, CredentialWrapper> m = [:]
-        environment.each {k, v ->


No longer compilable. Introduced in #147 but (AFAICT) without test coverage, and not clear it ever worked.

jglick · 2025-02-25T21:50:32Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-            return causeClass.class.simpleName.matches("(?i)\\.*${cause}.*")
+    public static boolean shouldRunBeAllowed(Cause causeClass, String cause, String detail) {
+        if (causeClass instanceof Cause.UserIdCause && Cause.UserIdCause.class.getSimpleName().equals(cause)) {
+            return detail == null || ((Cause.UserIdCause) causeClass).getUserId().equals(detail);
+        } else {
+            return causeClass.getClass().getSimpleName().matches("(?i).*" + cause + ".*");


Unclear what was meant by \.*cause.*. This would seem to mean mycauselala, .mycause, ..mycausexxx, etc.? Does not seem to make any sense. I think .* (any characters) was meant. Maybe .*\.?

(Copilot quietly converted the regexp, which is a bit alarming; I noticed the change while looking over the diff.)

Called via #isRunCausedBy by TriggeredByConditionalScript. So seems to support when { triggeredBy 'SCMTrigger' } etc.

What the pattern generally seems to do is it allows shorthand, e.g. triggeredBy 'SCM' (or 'scm' thanks to case-insensitivity). I don't see how a #getSimpleName would start with . though, so all this currently does is effectively ^ (no prefix).

The "fix" appears to change behavior (from matching a prefix to matching any substring) so should be done only carefully considered.

all this currently does is effectively ^ (no prefix)

Agreed. Maybe better to just write it that way for now.

jglick · 2025-02-25T21:52:28Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-        toApply.addAll(newProperties)
-        return toApply.asList()
+        if (newProperties != null) {
+            toApply.addAll(newProperties);


Copilot quietly inserted a null check here. The parameter is declared @CheckForNull so this might have been an NPE, unless it was in fact always non-null?

jglick · 2025-02-25T21:54:19Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-                    }
+    public static Predicate<FlowNode> isParallelBranchFlowNode(final String stageName) {
+        return input -> {
+            if (input != null) {


(ignore WS)

jglick · 2025-02-25T21:54:48Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

+        // Remove all triggers defined in previous Jenkinsfile and may be deprecated
+        toApply.removeIf(t -> prevDefined.contains(t.getDescriptor().getId()));
+        // Replace all triggers defined in current Jenkinsfile
+        if (newTriggers != null) {


… a `FlowExecution` argument

jglick · 2025-02-26T00:28:11Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

                                                           @NonNull List<JobProperty> existingProperties) {
-        Set<String> currentPropertiesDescriptors = currentProperties.collect{ it.descriptor.id }.toSet()
-        return existingProperties.findAll{ !(it.descriptor.id in currentPropertiesDescriptors)}
+        Set<String> currentPropertiesDescriptors = currentProperties != null ? currentProperties.stream()


Was able to resolve one SpotBugs violation here (currentProperties null check), but if I do the same in a nearby method, the diff goes over 50% modified: #765 (comment)

jglick · 2025-02-26T00:28:40Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-                countMap[it.descriptor.id] > 1 ||
-                !(Objects.equals(it, descriptorsToExistingProperties[it.descriptor.id]) ||
-                        isObjectsEqualsXStream(it, descriptorsToExistingProperties[it.descriptor.id]))}
+        return currentProperties.stream()


(currentProperties null check)

…ment)

dwnusbaum

Seems fine. The only thing that I noticed was that [:] has been converted to HashMap everywhere rather than LinkedHashMap. Whether this makes a meaningful difference anywhere was not immediately obvious to me.

jglick · 2025-02-26T15:39:29Z

...ine-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.java

-            return causeClass.class.simpleName.matches("(?i)\\.*${cause}.*")
+    public static boolean shouldRunBeAllowed(Cause causeClass, String cause, String detail) {
+        if (causeClass instanceof Cause.UserIdCause && Cause.UserIdCause.class.getSimpleName().equals(cause)) {
+            return detail == null || ((Cause.UserIdCause) causeClass).getUserId().equals(detail);
+        } else {
+            return causeClass.getClass().getSimpleName().matches("(?i)^" + cause + ".*");


#765 (comment)

There are three tests which exercise triggeredBy. All are testing positive matches, two exercising this class, both testing a positive match on the full Class.simpleName (no prefix or substring, no case insensitivity). There is also no help for this field;

pipeline-model-definition-plugin/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/TriggeredByConditional/help-compare.html

Lines 26 to 33 in 8bf54c8

Cause class simpleName case insensitive.

<li>

<ul>cause: TimerTrigger</ul>

<ul>cause: SCMTrigger</ul>

<ul>cause: UpstreamCause</ul>

<ul>cause: UserIdCause, detail: username</ul>

</li>

</p>

seems to describe the tested cases (with incorrect syntax) but is in the wrong filename so it does not appear in the GUI, while https://www.jenkins.io/doc/book/pipeline/syntax/#built-in-conditions covers the same limited text. Mistakes seem to all date to #306.

jglick · 2025-05-05T12:59:10Z

[:] has been converted to HashMap everywhere rather than LinkedHashMap

Hmm, good point, probably needs to be checked.

MappedClosure

f90061b

jglick added the chore label Feb 25, 2025

jglick added 2 commits February 25, 2025 15:29

Turns out Tools and Agent actually expected resultMap to be ass…

674b4a3

…ignable

Utils

50b57ad

jglick commented Feb 25, 2025

View reviewed changes

jglick added 3 commits February 25, 2025 18:15

Groovy collectEntries apparently merges so as to keep the last entry

66f7ff5

markStartAndEndNodesInStageAsNotExecuted is not in fact called with…

14cdf5c

… a `FlowExecution` argument

SpotBugs

dd54efa

jglick marked this pull request as ready for review February 26, 2025 00:18

jglick requested a review from a team February 26, 2025 00:18

jglick changed the title ~~Converting Groovy sources to Java~~ Converting some Groovy sources to Java Feb 26, 2025

jglick commented Feb 26, 2025

View reviewed changes

Alternate interpretation of probably faulty regexp jenkinsci#765 (com…

8bf54c8

…ment)

dwnusbaum approved these changes Feb 26, 2025

View reviewed changes

jglick commented Feb 26, 2025

View reviewed changes

jglick mentioned this pull request Mar 6, 2025

Bump org.jenkins-ci.plugins:plugin from 5.7 to 5.9 #775

Merged

joseblas approved these changes May 5, 2025

View reviewed changes

jglick marked this pull request as draft May 5, 2025 12:59

	Cause class simpleName case insensitive.
	<li>
	<ul>cause: TimerTrigger</ul>
	<ul>cause: SCMTrigger</ul>
	<ul>cause: UpstreamCause</ul>
	<ul>cause: UserIdCause, detail: username</ul>
	</li>
	</p>

Converting some Groovy sources to Java #765

Are you sure you want to change the base?

Converting some Groovy sources to Java #765

Conversation

jglick commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Feb 25, 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!

daniel-beck Feb 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented May 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jglick Feb 25, 2025 •

edited

Loading

daniel-beck Feb 26, 2025 •

edited

Loading