-
Notifications
You must be signed in to change notification settings - Fork 230
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.
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
[ATH-1036] Fix ath-container and docker based tests #1037
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the jenkins-infra point of view
Too many failures here, and I can not make sense of the error, I suspect it is because of an infra problem. On the good side it looks like more tests are run, I will retrigger this build by using an empty commit |
it looks like something has seriously regressed in either the LTS or was merged that broke pretty much all the tests. https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/master/683/testReport/ could not see any merges around this timeline and is mostly around save - given the nature of the first few tests I looked at... something is clearly wrong though. https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/master/683/testReport/junit/core/FreestyleJobTest/java_11_jenkins_lts_split4___discardBuilds/ if you look at the last screenshot you see a jenkins version that is not an LTS version. Could well be a bug in the junit attachment plugin but still.... |
Co-authored-by: James Nord <jtnord@users.noreply.github.com>
So, 56 skipped tests in this pr vs 226 in master -> 170 new tests run Also for example https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1037/4/testReport/plugins/WorkflowPluginTest/java_11_jenkins_latest_split6___hello_world_from_git/ is run in this pr, but in https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/master/684/testReport/plugins/WorkflowPluginTest/java_11_jenkins_lts_split8___hello_world_from_git/ is skipped because docker is not found. I do believe we can merge this, as failures are unrelated and the goal of unlocking docker tests is achieved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe we can merge this, as failures are unrelated and the goal of unlocking docker tests is achieved
Please do not merge this, a broken main build is the most important activity. Without a green baseline we cannot correctly evaluate this change and make sure there's no regression introduced, e.g. if this breaks any docker tests etc.
Filed #1039 which should be fixed before merging this. |
What makes you believe those now ignored docker tests will be green? Actually in proprietary runs using another infra most of those tests are failing because of changes already merged in I understand a green Or in other words, I do prefer a legit red build that I can fix with confidence step by step than trying to fix something that will be anyway broken without me knowing. Because right now any fix that I may propose to for example |
You (or anyone else) can branch from this PR to fix those as well. Merging on top of red is something I absolutely do not support. (I've ran out of time to check them for now but the next thing I would try it reverting #1024 to see if they go back to green) |
#1024 has proved blind faith in green builds is not a guarantee of success. But anyway I do not have more time to dedicate to this for the moment. If anyone wants to use this as base for future PRs please go forward. |
Seems lots of docker tests are failing for not being able to build the image currently |
Let me check, I had problems with old docker versions locally |
Not all tests are failing because of that, of my random 4 picks it was 50%, anyway, let's see what is failing |
Oh, Those kind of errors should be easy to fix or to die trying |
See refenrence in Dockerfile
So, subversion and (I hope) jira plugin should be fixed on the last run, next step git plugin tests |
|
As tomcat 7 is not java11 compatible
@@ -3,7 +3,7 @@ | |||
# and prepares for execution of gitplugin tests. | |||
# | |||
|
|||
FROM ubuntu:22.04 | |||
FROM ubuntu:xenial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most likely the line 24
and 19
are incorrect
RUN cat /home/git/unsafe.pub >> /home/git/.ssh/authorized_keys
is likely creating the file using the default keymask (infact the line 19 mkdir -p /home/git/.ssh
is probably just as bad.
the .ssh
folder should be 0700
and be owned by the git
user (not root)
line 30 RUN chown -R git /home/git
changes the ownership back but would not address any permission changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had some success with this suggestion locally, let's see what the pr builder says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with @jtnord . I'm not sure how this Java code works to spin up Docker container (and I'm too scared to try it), but I got the following updated Dockerfile here tested as below (as a first step) and passed through hadolint
(which has very very good tips and actionnables, like shellcheck
):
FROM ubuntu:22.04
SHELL ["/bin/bash", "-o", "pipefail", "-c"]
# install SSHD, Git and zip, always on the latest available version
# hadolint ignore=DL3008
RUN apt-get update \
&& apt-get install --yes --no-install-recommends \
ca-certificates \
openssh-server \
git \
zip \
&& rm -rf /var/lib/apt/lists/*
# create a git user and setup its .ssh dir (including public key)
COPY unsafe.pub /home/git/.ssh/authorized_keys
RUN useradd git -d /home/git \
&& echo "git:git" | chpasswd \
&& mkdir -p /var/run/sshd \
&& chmod 0700 /home/git/.ssh \
&& chmod 0600 /home/git/.ssh/* \
&& chown -R git /home/git
# run SSHD in the foreground with error messages to stderr
ENTRYPOINT ["/usr/sbin/sshd"]
CMD ["-D","-e"]
$ pwd
<my workspace>/acceptance-test-harness/src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitContainer
$ docker build -t sshd ./
# Success
$ docker run --rm --detach --name=sshd-test --publish 2222:22 sshd
# Run in background the container
$ ssh -i ./unsafe -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -p2222 git@localhost whoami
# ..
git
## It works
@timja @dduportal @jtnord Can you re review please? AFAICT the JobConfigHistoryTest is a flake, it has been working and failing randomly during the course of this PR while my changes should not affect it at all |
RUN chmod 700 /home/git/.ssh | ||
RUN chmod 600 /home/git/.ssh/authorized_keys | ||
|
||
RUN echo "HostbasedAcceptedKeyTypes +ssh-rsa\nHostKeyAlgorithms +ssh-rsa\nPubkeyAcceptedKeyTypes +ssh-rsa" >> /etc/ssh/sshd_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this for jsch to be able to sftp into the container, I tried with more modern algorithms for the keys but jsch did not supported them
|
||
@Test | ||
@Native("bash") | ||
@WithCredentials(credentialType = WithCredentials.USERNAME_PASSWORD, values = {"admin", "tomcat"}, id = "tomcat") | ||
public void deploy_sample_webapp_to_tomcat7() throws IOException, InterruptedException { | ||
public void deploy_sample_webapp_to_tomcat10() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe deploying in tomcat 10 vs deploying in tomcat 7 changes the intent of the test
You could file an issue and disable it if you don’t have time to look at it. |
Head branch was pushed to by a user without write access
Issues created, see link, and test ignored |
@timja Can you please reenable auto merge or merge if green? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 🚀
Caused #1322 |
See #1036 since #878 docker based tests are not run on ci.jenkins.io because
docker
can not be found on theath
image.I have updated the docker installation process according to https://docs.docker.com/engine/install/ubuntu/ and also reverted c54ef5f#diff-59086a3e54e38095246aa3a051f9020274dbf005b4652a7808cd4c6c239c536eL67 so
ath-user
can see and run the docker command.I have checked that the image builds and you can run docker inside as
ath-user
. Note that by running again docker tests we may find failures that were ignored before by accident. The purpose of this PR is not to solve those tests but to make the failures visible again. To check this is working we can compare the number of tests run for theWorkflowPluginTest
suite inmaster
and for this PR