Replace docker-fixtures with testcontainers#414
Replace docker-fixtures with testcontainers#414jglick merged 1 commit intojenkinsci:masterfrom strangelookingnerd:testcontainers
docker-fixtures with testcontainers#414Conversation
|
Once this change is merged and released it needs to be picked up by |
There was a problem hiding this comment.
This PR is introducing technical debt, as the Dockerfile is duplicated between #414 and jenkinsci/docker-fixtures#122, which creates a maintenance burden. See this page for more information about why this is the case. I recommend refactoring the common images that used to be present in docker-fixtures into a new Jenkins repository for TestContainers, something like https://github.com/dasniko/testcontainers-keycloak.
| 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, the comment and deprecation were in fact wrong.
|
I have thought of that but have mixed feelings. On the one hand, there would be less duplication. On the other hand, it makes it harder to develop and maintain tests because the repo is not self-contained: if you wish to adjust even a minor aspect of the Docker environment, you have to open a pull request on an external repo, spend time thinking about whether it is backward compatible, find someone to merge it, wait for it to be published, and only then proceed with your code changes. Another benefit of a published image is that tests would also run faster on fresh CI machines since they would only need to download an image, not build one in steps. This can be solved by using |
| 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")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
The duplication has a significant negative cost for doing Java version upgrades. Each time we have done a Java version upgrade in the past, a significant amount of time has been spent hunting down old Docker images in tests and updating them to include newer Java versions. With a common definition in
For the most part these images don't change very often, only a few times in many years (for Java version upgrades mostly), so I don't see this as a real downside. |
3 participants
Replace
docker-fixtureswithtestcontainers(see jenkinsci/docker-fixtures#122 (comment))This change removes already deprecated API methods from the test jar.
Testing done
mvn clean verfiySubmitter checklist