Converting SshAgentContainer & XvncSlaveContainer to Testcontainers#2582
Conversation
|
Maybe a flake? Just suspicious it would have appeared in this PR. Note that this test case does not use Docker and so should not have been affected by the fixture changes. |
see #2568 known issue, theory is timing related in JavaScript. Can be reproduced locally if you run it enough. @jtnord saw it 1/10 times. Sometimes I have seen it multiple times in a row. |
| return resource("ed25519.priv").asText(); | ||
| public SshAgentContainer() { | ||
| super(new ImageFromDockerfile("localhost/testcontainers/ath-ssh-agent", false) | ||
| .withFileFromClasspath(".", SshAgentContainer.class.getName().replace('.', '/'))); |
There was a problem hiding this comment.
NIT: generally you should only add the things that are needed - otherwise all of them can be sent to the testcontainer server for building (and it takes longer). (the build is currently only a local(ish) docker compliant server so currently this does not add too much overhead)
There was a problem hiding this comment.
generally you should only add the things that are needed
WDYM? This is sending https://github.com/jenkinsci/acceptance-test-harness/tree/5f032d5666d5e8e8fe0ccdf635d6a0866654c851/src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/SshAgentContainer which is exactly what is needed by the Docker daemon.
There was a problem hiding this comment.
generally you should only add the things that are needed
WDYM? This is sending https://github.com/jenkinsci/acceptance-test-harness/tree/5f032d5666d5e8e8fe0ccdf635d6a0866654c851/src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/SshAgentContainer which is exactly what is needed by the Docker daemon.
it only needs some of the files in that directory (the Dockerfile and the .pubs and the .pub is questionable) .
e.g.
// ...
.withFileFromClasspath("Dockerfile", SshAgentContainer.class.getName().replace('.', '/'))
.withFileFromClasspath("ed25519.pub", SshAgentContainer.class.getName().replace('.', '/') + "/ed25519.pub")
.withFileFromClasspath("unsafe_enc_key.pub", SshAgentContainer.class.getName().replace('.', '/') + "/unsafe_enc_key.pub")
//...There was a problem hiding this comment.
OK we could .dockerignore the private keys for example, but these are tiny files. We just do not want to be streaming megabytes of unused binaries, and we are not.
There was a problem hiding this comment.
hence was a NIT, not blocking anything on this
| public String getEncryptedEd25519PrivateKey() { | ||
| return resource("ed25519.priv").asText(); | ||
| public SshAgentContainer() { | ||
| super(new ImageFromDockerfile("localhost/testcontainers/ath-ssh-agent", false) |
There was a problem hiding this comment.
I thought not deleting the image would causes issues with stale images when the Dockerfile or other input was changed, but apparently there is something in testcontainers here that handles this. (could not find docs on it though)
There was a problem hiding this comment.
AFAIK specifying true (or not specifying it, e.g., just use the no-arg ctor) just means that it will do something like docker rmi after the test completes, throwing away your cache. Whereas passing false is the equivalent of just docker build.
There was a problem hiding this comment.
Confirmed: edits to Dockerfile are honored when you next mvn test.
There was a problem hiding this comment.
Confirmed: edits to
Dockerfileare honored when you nextmvn test.
sorry I was unclear - I had already tested this happened did not mean for you to test it, and this was a comment for anyone else who may see this in the future.
| echo 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzpxmTW9mH87DMkMSqBrSecoSHVCkKbW5IOO+4unak8M8cyn+b0iX07xkBn4hUJRfKA7ezUG8EX9ru5VinteqMOJOPknCuzmUS2Xj/WJdcq3BukBxuyiIRoUOXsCZzilR/DOyNqpjjI3iNb4los5//4aoKPCmLInFnQ3Y42VaimH1298ckEr4tRxsoipsEAANPXZ3p48gGwOf1hp56bTFImvATNwxMViPpqyKcyVaA7tXCBnEk/GEwb6MiroyHbS0VvBz9cZOpJv+8yQnyLndGdibk+hPbGp5iVAIsm28FEF+4FvlYlpBwq9OYuhOCREJvH9CxDMhbOXgwKPno9GyN kohsuke@atlas' > /home/test/.ssh/authorized_keys && \ | ||
| echo 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDagNSCDst/8z5oH9S5QWr+QNdx+haImY0FD3IQvKdD+eWI9zUbBgtoo/yYEbLvpTWiKsgT3Hw1F8mZ+/bd2Uv3lPyoG+TSzrHL4gSal6d1RWVjCOzSosciXVm4gRUvJjKXzaz8dOg+ii9yIrbeONNK0nlDUCAKy5YXSEl0avcPdUDyR3cStL6870SyanxAzktDw0n8xMq4F/alF3PZ002bcZJrmDeNVAwkP+uO2Tf8pN37SU+nApotZmlmZR32xYHnx+/OiQ7gOAVYmgNRMg0Kwh6Q73FcY3ZWCeNHwLnr95LoEAdj3On8Qr62VhGThuQNVCqBc6SeYjArfjijpcW9 jenkins-ci@localhost' >> /home/test/.ssh/authorized_keys && \ |
There was a problem hiding this comment.
Why do this as well as the following below (those public keys are identical are they not, and if not would it not be better to just manage them all in the same way to avoid confusion)?
COPY *.pub /tmp/
RUN cat /tmp/*.pub >> /home/test/.ssh/authorized_keys
There was a problem hiding this comment.
To try to keep this PR straightforward to review,
For now I just inlined the original Dockerfile contents; could be cleaned up in various ways later.
There was a problem hiding this comment.
IOW, this is making the minimum changes:
- changing Java 17 to 21
- switch from
docker-fixturesto exact equivalentDockerfilelayers
3 participants
Beginning of #857 (not converting other fixtures yet). Needed to make agents run on Java 21 (#2576 (comment)). Avoids the need for a new
docker-fixturesrelease (jenkinsci/docker-fixtures#138 (comment) jenkinsci/docker-fixtures#122 (comment)) and so supersedes #2581. Cannot just usejenkins/ssh-agentlike in jenkinsci/ssh-agents-plugin#984 since some of these tests rely on password authentication, whereas the official image only supports SSH private key authentication. For now I just inlined the originalDockerfilecontents; could be cleaned up in various ways later.