Introducing ControllerToAgentCallable and ControllerToAgentFileCallable

[jglick]

While updating some plugin code to take advantage of Java 17 records, I noticed that one major use case for a static final class with all final fields was MasterToSlaveCallables. Unfortunately these cannot be converted to record since that cannot extend another class. I thought it would be nicer to introduce a similar interface (with updated naming, while we are here!) that is friendlier to records.

I picked RemoteLaunchCallable as an example of substantial legibility improvements from this conversion, but could try doing more as well. (OpenRewrite would be great though I am not experienced in that yet.)

Similarly, MasterToSlaveFileCallable was unnecessarily awkward to use; FilePath.Archive can be simplified this way as an example.

Testing done

None, just presuming that RemoteLaunchCallable and FilePath.Archive are well covered by existing functional tests.

Note that the version of SpotBugs we are currently running suffers from spotbugs/spotbugs#2795 which is relevant for this use case: these records are Serializable (transiently), but no serialVersionUID is meaningful since Java serialization for records is handled specially. The SpotBugs check for jenkins-core does not seem to complain for whatever reason (maybe this bug priority is already suppressed?); in my plugin doing similar work I needed to

@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "TODO https://github.com/spotbugs/spotbugs/pull/2795 4.8.4+")

I do not feel very comfortable with the idea of reducing the typical idiom further to that using a static method as in jenkinsci/jenkins-test-harness#671 because production code might involve different Java versions (or even vendors!) between controller and agent and it is not obvious to me that this would be safe. Given

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Test;
public class XTest {
    @Test public void x() throws Exception {
        FI fi = XTest::m;
        assertThat(fi.ap(1), is(2));
        byte[] data;
        try (var baos = new ByteArrayOutputStream(); var oos = new ObjectOutputStream(baos)) {
            oos.writeObject(fi);
            data = baos.toByteArray();
        }
        Files.write(Path.of("/tmp/fi.ser"), data);
        Object o;
        try (var bais = new ByteArrayInputStream(data); var ois = new ObjectInputStream(bais)) {
            o = ois.readObject();
        }
        fi = (FI) o;
        assertThat(fi.ap(1), is(2));
    }
    private static int m(int x) {
        return x + 1;
    }
    @FunctionalInterface interface FI extends Serializable {
        int ap(int x);
    }
}

you can see a rather complex serial form

$ strings /tmp/fi.ser 
!java.lang.invoke.SerializedLambdaoa
implMethodKind[
capturedArgst
[Ljava/lang/Object;L
capturingClasst
Ljava/lang/Class;L
functionalInterfaceClasst
Ljava/lang/String;L
functionalInterfaceMethodNameq
"functionalInterfaceMethodSignatureq
	implClassq
implMethodNameq
implMethodSignatureq
instantiatedMethodTypeq
[Ljava.lang.Object;
XTest
XTest$FIt
(I)It
XTestt

Proposed changelog entries

• Developer: Introducing ControllerToAgentCallable and ControllerToAgentFileCallable with support for records in controller to agent callables

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. 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). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@Vlatombe @nevingeorgesunny @gbhat618

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

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. 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).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[jglick]

@daniel-beck in #4683 you added this as a setter rather than constructor parameter, perhaps out of confusion with the similarly-named field in Launcher which actually needs to be mutable?

[daniel-beck]

Maybe, the original code is by Wadeck and it's been far too long at this point. If this works, the setter is restricted, so any cleanup is fine.

[jglick]

the setter is restricted

Not that it needed to be: the class was private to begin with.

[Vlatombe]

Given that, should this class be deprecated to discourage any new usage, or hint existing ones to move to records?

[jglick]

I suppose it could be, though it feels like overkill to deprecate it merely because an aesthetically preferable alternative is now available.

[timja]

LGTM

---

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!