Skip to content

Replace docker-fixtures with testcontainers #414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jglick merged 1 commit into from
Jul 18, 2025
Merged

Replace docker-fixtures with testcontainers #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.test</groupId>
<artifactId>docker-fixtures</artifactId>
<version>200.v22a_e8766731c</version>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
<version>1.21.3</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
59 changes: 19 additions & 40 deletions src/test/java/org/jenkinsci/plugins/workflow/ArtifactManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collections;
Expand All @@ -74,14 +73,14 @@
import jenkins.util.VirtualFile;
import org.apache.commons.io.IOUtils;
import org.jenkinsci.plugins.workflow.flow.StashManager;
import org.jenkinsci.test.acceptance.docker.Docker;
import org.jenkinsci.test.acceptance.docker.DockerImage;
import org.jenkinsci.test.acceptance.docker.fixtures.JavaContainer;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;
import org.testcontainers.DockerClientFactory;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.images.builder.ImageFromDockerfile;

/**
* {@link #artifactArchiveAndDelete} and variants allow an implementation of {@link ArtifactManager} plus {@link VirtualFile} to be run through a standard gantlet of tests.
Expand All @@ -90,21 +89,18 @@

@Rule public JenkinsRule r = new JenkinsRule();
@Rule public LoggerRule logging = new LoggerRule();
private static DockerImage image;
@BeforeClass public static void doPrepareImage() throws Exception {
image = prepareImage();

private static GenericContainer<?> container;

@BeforeClass public static void doPrepareImage() {
container = prepareImage();
}

/**
* @deprecated Not actually used externally.
*/
@Deprecated
public static @CheckForNull DockerImage prepareImage() throws Exception {
Docker docker = new Docker();
if (!Functions.isWindows() && docker.isAvailable()) { // TODO: Windows agents on ci.jenkins.io have Docker, but cannot build the image.
return docker.build(JavaContainer.class);
public static @CheckForNull GenericContainer<?> prepareImage() {
Copy link
Member

@jglick jglick Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you introducing a new public method? As noted in the original Javadoc, this is no longer used outside the repo, or should not be anyway.

Copy link
Contributor Author

@strangelookingnerd strangelookingnerd Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. It would still be used in https://github.com/jenkinsci/artifact-manager-s3-plugin/pull/648/files#diff-3959b6e418b899f133a364de0b56605f10b1d8841d695a20235b01b8a0c28c1fR135-R136
Not 100% sure if this is actually required.

Copy link
Member

@jglick jglick Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, the comment and deprecation were in fact wrong.

if (!Functions.isWindows() && DockerClientFactory.instance().isDockerAvailable()) { // TODO: Windows agents on ci.jenkins.io have Docker, but cannot build the image.

Check warning on line 100 in src/test/java/org/jenkinsci/plugins/workflow/ArtifactManagerTest.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO 

NORMAL:
Windows agents on ci.jenkins.io have Docker, but cannot build the image.
return new GenericContainer<>(new ImageFromDockerfile("java17-ssh", false)
.withFileFromClasspath("Dockerfile", ArtifactManagerTest.class.getName().replace('.', '/') + "/Dockerfile"))
.withExposedPorts(22);
} else {
System.err.println("No Docker support; falling back to running tests against an agent in a process on the same machine.");
return null;
Expand All @@ -119,14 +115,13 @@
if (factory != null) {
ArtifactManagerConfiguration.get().getArtifactManagerFactories().add(factory);
}
JavaContainer runningContainer = null;
try {
DumbSlave agent;
if (image != null) {
runningContainer = image.start(JavaContainer.class).start();
if (container != null) {
container.start();
StandardUsernameCredentials creds = new UsernamePasswordCredentialsImpl(CredentialsScope.SYSTEM, "test", "desc", "test", "test");
CredentialsProvider.lookupStores(Jenkins.get()).iterator().next().addCredentials(Domain.global(), creds);
agent = new DumbSlave("test-agent", "/home/test/slave", new SSHLauncher(runningContainer.ipBound(22), runningContainer.port(22), "test"));
agent = new DumbSlave("test-agent", "/home/test/slave", new SSHLauncher(container.getHost(), container.getMappedPort(22), "test"));
Copy link
Member

@jglick jglick Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually recommend switching from this container to a standard one. https://hub.docker.com/r/jenkins/ssh-agent could probably be used, just passing a predefined JENKINS_AGENT_SSH_PUBKEY. It may be easier to switch to an inbound https://hub.docker.com/r/jenkins/agent though. https://github.com/jenkinsci/workflow-cps-plugin/blob/b2a1d7b29c6fdd0161dbb1c7586d579d49bbe1e2/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/SerialFormTest.java#L80-L85 works (though the requirement to exposeHostPorts is admittedly ugly); note that that test is more complex than you actually need for most purposes specifically because it is testing upgrade from @LocalData. I swear I wrote a simpler version somewhere recently, but I cannot find it now.

Copy link
Member

@jglick jglick Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, there are three common reasons to use containers in a Jenkins test:

  • To run some random service, like LDAP.
  • To run an agent with specific tools installed that it is expected to call, like a particular version of an SCM.
  • To run an agent but where the test does not care anything about the launcher or the details of the container image or even whether the agent runs in a container at all—merely that it not run in the same OS namespace as the controller (and test code), otherwise you could just use JenkinsRule.createAgent.

This test falls into the last category. The idea is to catch mistakes in remoted callables which might be presuming that a path valid on the controller is also valid on an agent or vice-versa. So we can use any inbound or outbound agent image from the past few years (just needs Java 17+ obviously) and it would be fine.

Copy link
Member

@basil basil Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second and third of these need a version of Java that is compatible with the controller, and every few years this changes, so it is important that we not duplicate too much code here if we can avoid it. Relying on the standard https://hub.docker.com/r/jenkins/ssh-agent sounds great to me, but it is possibly more effort to migrate tests to that approach. A more simple and direct port of the existing logic (and therefore lower-effort migration) would be to simply convert the existing images in this repository to Test Containers along the lines of https://github.com/dasniko/testcontainers-keycloak. That would still preserve the existing tech debt of having a test image distinct from the production image, but it would still reduce the test debt of using an unmaintained framework (docker-fixtures) in favor of a maintained framework (Test Containers), without having to duplicate the image in each consumer.

Copy link
Member

@jglick jglick Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an option of course, but we would anyway want to throw out any such images sooner or later and just use the standard jenkins/{,ssh-}agent images for these cases of the third type. Since the existing docker-fixtures release is working fine as it is, I do not see any urgency in an intermediate fix: we may as well write tests the straightforward way to begin with. I can check feasibility in this PR next week.

Copy link
Member

@jglick jglick Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swear I wrote a simpler version somewhere recently, but I cannot find it now.

Ah found it at last: jenkinsci/docker-workflow-plugin#351 (comment)

Jenkins.get().addNode(agent);
r.waitOnline(agent);
} else {
Expand All @@ -142,8 +137,8 @@
FreeStyleBuild b = r.buildAndAssertSuccess(p);
f.apply(agent, p, b, ws);
} finally {
if (runningContainer != null) {
runningContainer.close();
if (container != null) {
container.stop();
}
}
}
Expand All @@ -161,10 +156,6 @@
assertTrue(b.getArtifactManager().root().child("file").isFile());
});
}
@Deprecated
public static void artifactArchive(@NonNull JenkinsRule r, @CheckForNull ArtifactManagerFactory factory, boolean weirdCharacters, @CheckForNull DockerImage image) throws Exception {
artifactArchive(r, factory, weirdCharacters);
}

/**
* Test artifact archiving in a plain manager.
Expand All @@ -180,10 +171,6 @@
assertFalse(b.getArtifactManager().delete());
});
}
@Deprecated
public static void artifactArchiveAndDelete(@NonNull JenkinsRule r, @CheckForNull ArtifactManagerFactory factory, boolean weirdCharacters, @CheckForNull DockerImage image) throws Exception {
artifactArchiveAndDelete(r, factory, weirdCharacters);
}

/**
* Test stashing and unstashing with a {@link StashManager.StashAwareArtifactManager} that does <em>not</em> honor deletion requests.
Expand All @@ -197,10 +184,6 @@
assertTrue(ws.child("file").exists());
}));
}
@Deprecated
public static void artifactStash(@NonNull JenkinsRule r, @CheckForNull ArtifactManagerFactory factory, boolean weirdCharacters, @CheckForNull DockerImage image) throws Exception {
artifactStash(r, factory, weirdCharacters);
}

/**
* Test stashing and unstashing with a {@link StashManager.StashAwareArtifactManager} with standard behavior.
Expand All @@ -218,10 +201,6 @@
assertFalse(ws.child("file").exists());
}));
}
@Deprecated
public static void artifactStashAndDelete(@NonNull JenkinsRule r, @CheckForNull ArtifactManagerFactory factory, boolean weirdCharacters, @CheckForNull DockerImage image) throws Exception {
artifactStashAndDelete(r, factory, weirdCharacters);
}

/**
* Creates a variety of files in a directory structure designed to exercise interesting aspects of {@link VirtualFile}.
Expand Down Expand Up @@ -471,7 +450,7 @@
@Test public void standard() throws Exception {
logging.record(StandardArtifactManager.class, Level.FINE);
// Who knows about weird characters on NTFS; also case-sensitivity could confuse things
artifactArchiveAndDelete(r, null, !Functions.isWindows(), image);
artifactArchiveAndDelete(r, null, !Functions.isWindows());
}

}
25 changes: 25 additions & 0 deletions src/test/resources/org/jenkinsci/plugins/workflow/ArtifactManagerTest/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
FROM ubuntu:noble

# install requirements
RUN apt-get update -y && \
apt-get install --no-install-recommends -y \
openjdk-17-jdk-headless \
openssh-server \
locales

RUN mkdir -p /var/run/sshd

# create a test user
RUN useradd test -d /home/test && \
mkdir -p /home/test/.ssh && \
chown -R test:test /home/test && \
echo "test:test" | chpasswd

# https://stackoverflow.com/a/38553499/12916
RUN sed -i -e 's/# en_US.UTF-8 UTF-8/en_US.UTF-8 UTF-8/' /etc/locale.gen && \
dpkg-reconfigure --frontend=noninteractive locales && \
update-locale LANG=en_US.UTF-8
ENV LANG en_US.UTF-8

# run SSHD in the foreground with error messages to stderr
ENTRYPOINT ["/usr/sbin/sshd", "-D", "-e"]