[JENKINS-48761] - Address review comments from @jglick

jenkinsci · Jan 3, 2018 · 01b8ed8 · 01b8ed8
1 parent 8a34983
commit 01b8ed8
Show file tree

Hide file tree

Showing 13 changed files with 31 additions and 39 deletions.
diff --git a/core/src/filter/resources/jenkins/slaves/remoting-info.properties b/core/src/filter/resources/jenkins/slaves/remoting-info.properties
@@ -2,5 +2,5 @@
 # This version MAY differ from what is really classloaded (see the "Pluggable Core Components", JENKINS-41196)
 remoting.embedded.version=${remoting.version}
 
-# Minimal Remoting version on external agents which is supported by the core
-remoting.minimal.supported.version=${remoting.minimal.supported.version}
+# Minimum Remoting version on external agents which is supported by the core
+remoting.minimum.supported.version=${remoting.minimum.supported.version}
diff --git a/core/src/main/java/hudson/Launcher.java b/core/src/main/java/hudson/Launcher.java
@@ -44,7 +44,6 @@
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 import javax.annotation.CheckForNull;
-import javax.annotation.concurrent.GuardedBy;
 import java.io.BufferedOutputStream;
 import java.io.File;
 import java.io.IOException;
@@ -1289,7 +1288,7 @@ private static class RemoteLaunchCallable extends MasterToSlaveCallable<RemotePr
         }
 
         public RemoteProcess call() throws IOException {
-            final Channel channel = _getOpenChannelOrFail();
+            final Channel channel = getOpenChannelOrFail();
             Launcher.ProcStarter ps = new LocalLauncher(listener).launch();
             ps.cmds(cmd).masks(masks).envs(env).stdin(in).stdout(out).stderr(err).quiet(quiet);
             if(workDir!=null)   ps.pwd(workDir);

diff --git a/core/src/main/java/hudson/TcpSlaveAgentListener.java b/core/src/main/java/hudson/TcpSlaveAgentListener.java
@@ -290,7 +290,7 @@ private void respondHello(String header, Socket s) throws IOException {
             try {
                 Writer o = new OutputStreamWriter(s.getOutputStream(), "UTF-8");
 
-                //TODO: expose version about minimal supported Remoting version (JENKINS-48766)
+                //TODO: expose version about minimum supported Remoting version (JENKINS-48766)
                 if (header.startsWith("GET / ")) {
                     o.write("HTTP/1.0 200 OK\r\n");
                     o.write("Content-Type: text/plain;charset=UTF-8\r\n");

diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java
@@ -32,7 +32,6 @@
 import hudson.slaves.Cloud;
 import jenkins.util.SystemProperties;
 import hudson.Util;
-import hudson.cli.declarative.CLIMethod;
 import hudson.cli.declarative.CLIResolver;
 import hudson.console.AnnotatedLargeText;
 import hudson.init.Initializer;
@@ -85,7 +84,6 @@
 import org.kohsuke.stapler.WebMethod;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
-import org.kohsuke.args4j.Option;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.annotation.OverridingMethodsMustInvokeSuper;
@@ -1426,7 +1424,7 @@ public void doDumpExportTable( StaplerRequest req, StaplerResponse rsp ) throws
 
     private static final class DumpExportTableTask extends MasterToSlaveCallable<String,IOException> {
         public String call() throws IOException {
-            final Channel ch = _getChannelOrFail();
+            final Channel ch = getChannelOrFail();
             StringWriter sw = new StringWriter();
             try (PrintWriter pw = new PrintWriter(sw)) {
                 ch.dumpExportTable(pw);

diff --git a/core/src/main/java/hudson/slaves/ChannelPinger.java b/core/src/main/java/hudson/slaves/ChannelPinger.java
@@ -134,7 +134,7 @@ public void install(Channel channel) {
         @Override
         public Void call() throws IOException {
             // No sense in setting up channel pinger if the channel is being closed
-            setUpPingForChannel(_getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false);
+            setUpPingForChannel(getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false);
             return null;
         }
 

diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java
@@ -44,7 +44,6 @@
 import hudson.security.ACL;
 import hudson.slaves.OfflineCause.ChannelTermination;
 import hudson.util.Futures;
-import hudson.util.IOUtils;
 import hudson.util.NullStream;
 import hudson.util.RingBufferLogHandler;
 import hudson.util.StreamTaskListener;
@@ -869,7 +868,7 @@ public Void call() {
             }
 
             try {
-                _getChannelOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side.
+                getChannelOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side.
             } catch (ChannelClosedException e) {
                 throw new IllegalStateException(e);
             }

diff --git a/core/src/main/java/jenkins/security/MasterToSlaveCallable.java b/core/src/main/java/jenkins/security/MasterToSlaveCallable.java
@@ -7,8 +7,6 @@
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
-import javax.annotation.Nonnull;
-
 
 /**
  * Convenient {@link Callable} meant to be run on agent.
@@ -25,21 +23,21 @@ public void checkRoles(RoleChecker checker) throws SecurityException {
         checker.check(this,Roles.SLAVE);
     }
 
-    //TODO: replace by Callable#getChannelOrFail() once Minimal supported Remoting version is 3.15 or above
-    @Restricted(NoExternalUse.class)
-    protected static Channel _getChannelOrFail() throws ChannelClosedException {
+    //TODO: remove once Minimum supported Remoting version is 3.15 or above
+    @Override
+    public Channel getChannelOrFail() throws ChannelClosedException {
         final Channel ch = Channel.current();
         if (ch == null) {
             throw new ChannelClosedException(new IllegalStateException("No channel associated with the thread"));
         }
         return ch;
     }
 
-    //TODO: replace by Callable#getOpenChannelOrFail() once Minimal supported Remoting version is 3.15 or above
-    @Restricted(NoExternalUse.class)
-    protected static Channel _getOpenChannelOrFail() throws ChannelClosedException {
-        final Channel ch = _getChannelOrFail();
-        if (ch.isClosingOrClosed()) { // TODO: Since Remoting 2.33, we still need to explicitly declare minimal Remoting version
+    //TODO: remove once Callable#getOpenChannelOrFail() once Minimaumsupported Remoting version is 3.15 or above
+    @Override
+    public Channel getOpenChannelOrFail() throws ChannelClosedException {
+        final Channel ch = getChannelOrFail();
+        if (ch.isClosingOrClosed()) { // TODO: Since Remoting 2.33, we still need to explicitly declare minimum Remoting version
             throw new ChannelClosedException(new IllegalStateException("The associated channel " + ch + " is closing down or has closed down", ch.getCloseRequestCause()));
         }
         return ch;

diff --git a/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java b/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java
@@ -37,9 +37,8 @@
 
 // TODO: Make the API public (JENKINS-48766)
 /**
- * Provides information about Remoting versions used withing the core.
+ * Provides information about Remoting versions used within the core.
  * @author Oleg Nenashev
- * @since TODO
  */
 @Restricted(NoExternalUse.class)
 public class RemotingVersionInfo {
@@ -51,7 +50,7 @@ public class RemotingVersionInfo {
     private static VersionNumber EMBEDDED_VERSION;
 
     @CheckForNull
-    private static VersionNumber MINIMAL_SUPPORTED_VERSION;
+    private static VersionNumber MINIMUM_SUPPORTED_VERSION;
 
     private RemotingVersionInfo() {}
 
@@ -66,7 +65,7 @@ private RemotingVersionInfo() {}
         }
 
         EMBEDDED_VERSION = tryExtractVersion(props, "remoting.embedded.version");
-        MINIMAL_SUPPORTED_VERSION = tryExtractVersion(props, "remoting.minimal.supported.version");
+        MINIMUM_SUPPORTED_VERSION = tryExtractVersion(props, "remoting.minimum.supported.version");
     }
 
     @CheckForNull
@@ -98,7 +97,7 @@ public static VersionNumber getEmbeddedVersion() {
     }
 
     @CheckForNull
-    public static VersionNumber getMinimalSupportedVersion() {
-        return MINIMAL_SUPPORTED_VERSION;
+    public static VersionNumber getMinimumSupportedVersion() {
+        return MINIMUM_SUPPORTED_VERSION;
     }
 }
diff --git a/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java b/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java
@@ -39,7 +39,7 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l
     private static final class ChannelSwapper extends MasterToSlaveCallable<Boolean,Exception> {
         public Boolean call() throws Exception {
             if (File.pathSeparatorChar==';')    return false;   // Windows
-            Channel c = _getOpenChannelOrFail();
+            Channel c = getOpenChannelOrFail();
             StandardOutputStream sos = (StandardOutputStream) c.getProperty(StandardOutputStream.class);
             if (sos!=null) {
                 swap(sos);

diff --git a/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java b/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java
@@ -40,8 +40,8 @@ public void shouldLoadEmbeddedVersionByDefault() {
     }
 
     @Test
-    public void shouldLoadMinimalSupportedVersionByDefault() {
-        assertThat("Remoting Minimal supported version is not defined",
-                RemotingVersionInfo.getMinimalSupportedVersion(), notNullValue());
+    public void shouldLoadMinimumSupportedVersionByDefault() {
+        assertThat("Remoting Minimum supported version is not defined",
+                RemotingVersionInfo.getMinimumSupportedVersion(), notNullValue());
     }
 }
diff --git a/pom.xml b/pom.xml
@@ -104,9 +104,9 @@ THE SOFTWARE.
 
     <maven-war-plugin.version>3.0.0</maven-war-plugin.version> <!-- JENKINS-47127 bump when 3.2.0 is out. Cf. MWAR-407 -->
 
-    <!-- Minimal Remoting version, which is tested for API compatibility -->
+    <!-- Minimum Remoting version, which is tested for API compatibility -->
     <remoting.version>3.15</remoting.version>
-    <remoting.minimal.supported.version>2.60</remoting.minimal.supported.version>
+    <remoting.minimum.supported.version>2.60</remoting.minimum.supported.version>
 
   </properties>
 

diff --git a/test/pom.xml b/test/pom.xml
@@ -239,7 +239,7 @@ THE SOFTWARE.
                 <artifactItem>
                   <groupId>org.jenkins-ci.main</groupId>
                   <artifactId>remoting</artifactId>
-                  <version>${remoting.minimal.supported.version}</version>
+                  <version>${remoting.minimum.supported.version}</version>
                   <type>jar</type>
                   <outputDirectory>${project.build.outputDirectory}/old-remoting</outputDirectory>
                   <destFileName>remoting-minimal-supported.jar</destFileName>

diff --git a/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java b/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java
@@ -44,7 +44,6 @@
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.SimpleCommandLauncher;
 
-import javax.annotation.CheckForNull;
 import java.io.File;
 import java.io.IOException;
 import java.lang.reflect.Method;
@@ -70,7 +69,7 @@ public class OldRemotingAgentTest {
     @Before
     public void extractAgent() throws Exception {
         agentJar = new File(tmpDir.getRoot(), "old-agent.jar");
-        FileUtils.copyURLToFile(getClass().getResource("/old-remoting/remoting-minimal-supported.jar"), agentJar);
+        FileUtils.copyURLToFile(OldRemotingAgentTest.class.getResource("/old-remoting/remoting-minimal-supported.jar"), agentJar);
     }
 
     @Test
@@ -81,7 +80,7 @@ public void shouldBeAbleToConnectAgentWithMinimalSupportedVersion() throws Excep
         boolean isUnix = agent.getComputer().isUnix();
         assertThat("Received wrong agent version. A minimal supported version is expected",
                 agent.getComputer().getSlaveVersion(),
-                equalTo(RemotingVersionInfo.getMinimalSupportedVersion().toString()));
+                equalTo(RemotingVersionInfo.getMinimumSupportedVersion().toString()));
 
         // Just ensure we are able to run something on the agent
         FreeStyleProject project = j.createFreeStyleProject("foo");
@@ -122,8 +121,8 @@ static void assertMonitor(NodeMonitor monitor, Computer c) throws AssertionError
             try {
                 monitorMethod = AbstractAsyncNodeMonitorDescriptor.class.getDeclaredMethod("monitor", Computer.class);
             } catch (NoSuchMethodException ex) {
-                System.out.println("Cannot invoke monitor " + monitor + ", no monitor(Computer.class) method in the Descriptor. It will be ignored. " + ex.getMessage());
-                return;
+                //TODO: make the API visible for testing?
+                throw new AssertionError("Cannot invoke monitor " + monitor + ", no monitor(Computer.class) method in the Descriptor. It will be ignored. ", ex);
             }
             try {
                 monitorMethod.setAccessible(true);