[JENKINS-69153] Do not call deprecated h.singletonList

[MarkEWaite]

JENKINS-69153 Do not call deprecated h.singletonList

Jenkins 2.358 deprecated the Functions.singletonList method because it is not normally needed in JEXL. The h.singletonList(it) call in this groovy code reports a null pointer exception in Jenkins 2.358 and later.

When the h.singletonList(it) call is replaced with it, the null pointer exception is not reported and the page loads.

Needs more testing from people that are experienced with email extension template plugin.

Would benefit from a round trip test that checks the page loads.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[jglick]

I have a hard time seeing why jenkinsci/jenkins#6740 would have introduced an NPE. Adding @Deprecated should not change any runtime behavior. The patch to Computer/sidepanel.jelly of course does, but why would that affect some other sidepanel view? Did you actually verify that the core commit causes the reported error, or could it be caused by some unrelated change?

[jglick]

java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:221)
	at java.base/java.util.ImmutableCollections$List12.<init>(ImmutableCollections.java:371)
	at java.base/java.util.List.of(List.java:807)
	at hudson.Functions.singletonList(Functions.java:1947)

from https://issues.jenkins.io/browse/JENKINS-69707 would seem to implicate jenkinsci/jenkins#6700, also in 2.358: Collections.singletonList(null) is legal whereas List.of((Object) null) for some reason is not.

[basil]

If this is the only known case where someone was trying to pass null to Functions#singletonList, and it is now fixed in this PR, then I would prefer to keep the stricter behavior in Functions#singletonList. If there are other callers that are passing null to Functions#singletonList, then I would be OK with reverting the Functions#singletonList portion of jenkinsci/jenkins#6700. The method is deprecated anyway, so it exists only for compatibility purposes and there is an argument for being less strict in that context.

[jglick]

I would prefer to keep the stricter behavior

Seems reasonable. I would just check why this plugin was trying to pass a list consisting of one null element to begin with—it looks like a mistake.

[MarkEWaite]

Seems reasonable. I would just check why this plugin was trying to pass a list consisting of one null element to begin with—it looks like a mistake.

That is a very good point. The side panel displayed by the plugin seems to be attempting to show the list of executors in an expanding and shrinking box as is done on the main dashboard and on the system info page http://localhost:8080/jenkins/manage/systemInfo page and the http://localhost:8080/jenkins/manage/cli/ page. However, on 2.361.1, even with this change, it still does not show the list of executors.

Would it be simpler to switch sidepanel.groovy to be a sidepanel.jelly and have it use a similar implementation to those other pages?

[jglick]

even with this change, it still does not show the list of executors

But it did before 2.358? Was it set to something useful? #71 (comment)

[MarkEWaite]

I was being too impatient with my i7 based Windows machine with 32 GB of RAM. The executor list appears in the editable email template page but it takes a noticeably longer time the executor list from the system properties or cli pages.

If I wait patiently, then the executor list appears. The delay is surprising, but much less of an issue than the exception that is shown by the current release of the plugin.

Editable email template page

late page for first 1-2 seconds

CLI page

System properties page

[jglick]

Yeah but it does not actually show any executors.

Anyway this widget does not make any sense here, looking at the screenshot. So the line should simply be deleted, as I suggested in #71 (comment).

[MarkEWaite]

@slide would you be OK if I released a version of this plugin from the master branch so that this fix is available to users? Comments in the bug report confirm that the fix is an improvement over the current state.

[slide]

Go for it, you can do that whenever needed