Disable by default; introduce dedicated test class; tone down logging

jglick · jglick · commit 2a9b72cd4e95 · 2026-07-13T17:57:00.000-04:00
diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java
@@ -174,7 +174,7 @@ public class KubernetesCloud extends Cloud implements PodTemplateGroup {
     private GarbageCollection garbageCollection;
 
     /** @see ContainerListenDecorator */
-    private boolean activeContainers = true; // TODO do not enable by default
+    private boolean activeContainers;
 
     @NonNull
     private DescribableList<KubernetesCloudTrait, KubernetesCloudTraitDescriptor> traits =
diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerListenDecorator.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerListenDecorator.java
@@ -72,7 +72,7 @@ final class ContainerListenDecorator extends LauncherDecorator implements Serial
     @Override
     public Launcher decorate(Launcher launcher, Node node) {
         if (!launcher.isUnix()) {
-            LOGGER.info(() -> "TODO using original implementation on Windows " + node.getNodeName());
+            LOGGER.fine(() -> "using original implementation on Windows " + node.getNodeName());
             return fallback.decorate(launcher, node);
         }
         if (!(node instanceof KubernetesSlave ks)) {
@@ -109,7 +109,7 @@ public Launcher decorate(Launcher launcher, Node node) {
                     x,
                     () -> "Could not verify eligibility of container " + container + " in " + ks.getPodName());
         }
-        LOGGER.info(() -> "TODO prepping launcher for " + container + " in " + ks.getPodName());
+        LOGGER.finer(() -> "prepping launcher for " + container + " in " + ks.getPodName());
         return new LauncherImpl(launcher, ks, pod);
     }
 
@@ -142,7 +142,7 @@ private class LauncherImpl extends Launcher.DecoratedLauncher {
         @Override
         public Proc launch(Launcher.ProcStarter starter) throws IOException {
             if (!isUnix()) {
-                throw new AbortException("TODO Windows not yet supported");
+                throw new AbortException("Windows not yet supported");
             }
             FilePath procDir;
             try {
@@ -157,7 +157,7 @@ public Proc launch(Launcher.ProcStarter starter) throws IOException {
                 if (!bootstrap.exists()) {
                     work.mkdirs();
                     bootstrap.copyFrom(ContainerListenDecorator.class.getResource("scripts/container-bootstrap.sh"));
-                    LOGGER.info("TODO created " + bootstrap);
+                    LOGGER.fine(() -> "created " + bootstrap);
                 }
                 procDir = work.child(container)
                         .child("%016x".formatted(RandomGenerator.getDefault().nextLong()));
@@ -186,7 +186,7 @@ public Proc launch(Launcher.ProcStarter starter) throws IOException {
                         .findFirst()
                         .orElse(null);
                 if (workspaceToContainer != null && !workspaceToContainer.equals(workspaceToAgent)) {
-                    LOGGER.info("TODO translating " + workspaceToAgent + " to " + workspaceToContainer);
+                    LOGGER.fine(() -> "translating " + workspaceToAgent + " to " + workspaceToContainer);
                     if (pwd.startsWith(workspaceToAgent + "/")) {
                         pwd = workspaceToContainer + pwd.substring(workspaceToAgent.length());
                     }
@@ -225,7 +225,7 @@ public Proc launch(Launcher.ProcStarter starter) throws IOException {
                     sb.append(" ");
                 }
                 f.write(sb.toString(), null);
-                LOGGER.info("TODO wrote to " + f + ": \n" + sb);
+                LOGGER.fine(() -> "wrote to " + f + ": \n" + sb);
                 if (!starter.quiet()) {
                     maskedPrintCommandLine(
                             cmds, starter.masks(), pwd != null ? new FilePath(pwdF.getChannel(), pwd) : null);
@@ -248,17 +248,20 @@ public void kill() throws IOException, InterruptedException {
                 @Override
                 public int join() throws IOException, InterruptedException {
                     var status = procDir.child("status.txt");
-                    LOGGER.info("TODO waiting for " + status);
+                    LOGGER.fine(() -> "waiting for " + status);
                     while (!status.exists()) {
                         Thread.sleep(100);
                     }
+                    var r = Integer.parseInt(status.readToString().trim());
                     // No streaming supported, just logging/capturing of stdio from a completed process.
                     // In practice most steps are durable, which produce no stdio themselves;
                     // or relatively brief and so streaming is unnecessary.
-                    LOGGER.info("TODO " + procDir + " completed with status "
-                            + status.readToString().trim() + " and stdout "
-                            + procDir.child("out.txt").length() + "b stderr"
-                            + procDir.child("err.txt").length() + "b");
+                    if (LOGGER.isLoggable(Level.FINE)) {
+                        LOGGER.fine(procDir + " completed with status "
+                                + status.readToString().trim() + " and stdout "
+                                + procDir.child("out.txt").length() + "b stderr"
+                                + procDir.child("err.txt").length() + "b");
+                    }
                     var os = starter.stdout();
                     if (os == null) {
                         os = getListener().getLogger();
@@ -269,7 +272,6 @@ public int join() throws IOException, InterruptedException {
                         os = getListener().getLogger();
                     }
                     procDir.child("err.txt").copyTo(new CloseProofOutputStream(os));
-                    var r = Integer.parseInt(status.readToString().trim());
                     procDir.deleteRecursive();
                     return r;
                 }
@@ -340,7 +342,6 @@ public static final class Decorator implements PodDecorator {
         @Override
         public Pod decorate(KubernetesCloud kubernetesCloud, Pod pod) {
             if (kubernetesCloud.isActiveContainers()) {
-                // TODO exclude Windows
                 for (var c : pod.getSpec().getContainers()) {
                     var cmds = c.getCommand();
                     if (cmds != null && !cmds.isEmpty() && cmds.get(0).matches("((/usr)?/bin/)?(sleep|cat)")) {
diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly
@@ -122,7 +122,7 @@ THE SOFTWARE.
     <f:optionalProperty field="garbageCollection" title="${%Enable garbage collection}"/>
 
     <f:entry title="${%Use active containers}" field="activeContainers">
-      <f:checkbox default="true"/> <!-- TODO do not enable by default -->
+      <f:checkbox/>
     </f:entry>
 
     <f:descriptorList field="traits" descriptors="${descriptor.allTraits}" />
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineTest.java
@@ -57,6 +57,7 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.LogRecorder;
 import org.jvnet.hudson.test.junit.jupiter.BuildWatcherExtension;
@@ -68,7 +69,7 @@ public abstract class AbstractKubernetesPipelineTest {
     protected static final String POD_ENV_VAR_VALUE = "pod-env-var-value";
     protected static final String GLOBAL = "GLOBAL";
 
-    @SuppressWarnings("unused")
+    @RegisterExtension
     protected static final BuildWatcherExtension BUILD_WATCHER = new BuildWatcherExtension();
 
     protected KubernetesCloud cloud;
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorPipelineTest.java
@@ -19,7 +19,6 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.*;
-import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
 import com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey;
 import com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey.PrivateKeySource;
@@ -97,7 +96,6 @@ void docker() throws Exception {
     @Issue("JENKINS-58290")
     @Test
     void closedWebSocketExit() throws Exception {
-        assumeFalse(cloud.isActiveContainers());
         assertNotNull(createJobThenScheduleRun());
         containerExecLogs.capture(1000);
         r.waitForMessage("have started user process", b);
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerListenDecoratorTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerListenDecoratorTest.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2026 CloudBees, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.csanchez.jenkins.plugins.kubernetes.pipeline;
+
+import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.deletePods;
+import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.getLabels;
+
+import java.util.logging.Level;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class ContainerListenDecoratorTest extends AbstractKubernetesPipelineTest {
+
+    @BeforeEach
+    void beforeEach() throws Exception {
+        deletePods(cloud.connect(), getLabels(cloud, this, name), false);
+        cloud.setActiveContainers(true);
+        logs.record(ContainerListenDecorator.class, Level.FINE);
+    }
+
+    @Test
+    void activeContainerSmokes() throws Exception {
+        r.assertLogContains(
+                "go version go1.6.3", r.assertBuildStatusSuccess(r.waitForCompletion(createJobThenScheduleRun())));
+    }
+
+    // TODO missing test coverage (compare https://github.com/jenkinsci/kubernetes-plugin/pull/2837/commits):
+    // · using cat rather than sleep
+    // · environment variables from agent pod vs. container
+    //   (KubernetesDeclarativeAgentTest#declarative & ContainerExecDecoratorPipelineTest#containerEnvironmentIsHonored)
+    // · BASH_COMPLIANT_ENV_VAR (KubernetesPipelineTest#runWithEnvVariablesInContext)
+    // · interrupting sh step (KubernetesPipelineTest#interruptedPod)
+    // · nonexistent container name (KubernetesDeclarativeAgentTest.declarativeWithNestedExplicitInheritance)
+    // · unpatched container
+    // · custom workingDir
+    // · restart controller during or between sh steps
+    // · container step used in addition to defaultContainer (so nested steps)
+    // · command masking (ContainerExecDecoratorPipelineTest.docker)
+    // · nondefault shell (KubernetesPipelineTest#runInPodWithDifferentShell but a passing build)
+    // · fallback on Windows
+
+}
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java
@@ -42,7 +42,6 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.oneOf;
 import static org.junit.jupiter.api.Assertions.*;
-import static org.junit.jupiter.api.Assumptions.assumeFalse;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import edu.umd.cs.findbugs.annotations.NonNull;
@@ -322,7 +321,6 @@ void runInPodWithDifferentShell() throws Exception {
         // Completed /home/jenkins/agent/container-work/busybox/1bfd9327891af5c0 with status 127
         // The hack used to make this pass in ContainerExecDecorator is unsatisfactory;
         // the proper fix should be in BourneShellScript like https://github.com/jenkinsci/durable-task-plugin/pull/561
-        assumeFalse(cloud.isActiveContainers());
         r.assertBuildStatus(Result.FAILURE, r.waitForCompletion(b));
         r.assertLogContains("ERROR: Process exited immediately after creation", b);
         // r.assertLogContains("/bin/bash: no such file or directory", b); // Not printed in CI for an unknown reason.
diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/activeContainerSmokes.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/activeContainerSmokes.groovy
@@ -0,0 +1,16 @@
+podTemplate(yaml: '''
+spec:
+  containers:
+  - name: golang
+    image: golang:1.6.3-alpine
+    command:
+    - sleep
+    args:
+    - infinity
+''') {
+  node(POD_LABEL) {
+    container('golang') {
+      sh 'go version'
+    }
+  }
+}
