-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fix: mount Docker credentials #17798
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
@@ -10,6 +10,7 @@ services: | |||
volumes: | |||
- ${PWD}/../..:/go/src/github.com/elastic/beats/ | |||
- /var/run/docker.sock:/var/run/docker.sock | |||
- ${HOME}/.docker:/root/.docker:ro |
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.
Fun fact, PWD
and HOME
are not available in docker-compose on windows. If we want to be able to run full system tests in the future on windows as well, we will have to do some modifications.
Does this impact developers wanting to test locally on their laptop?
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.
yep both env vars impact developers on local laptops that are testing x-pack/metricbeat
, so there were impacted by ${PWD}
before this PR, and not they are impacted too by ${HOME}
.
Also, any windows developer running Docker configured for windows containers cannot run the tests (windows only allow to install windows containers support or Linux containers support not both).
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.
Also, any windows developer running Docker configured for windows containers cannot run the tests (windows only allow to install windows containers support or Linux containers support not both).
Right. But docker-compose is evaluated from the POV of the host. So it doesn't work with linux containers on a windows host (I tried :) ). But this is not new. Replacing PWD
with .
helps btw.
The main impact is not ${PWD}
or ${HOME}
per se. But developers will need to create a .docker
file/directory. What shall we include there in order to test locally?
Alternatively we could introduce an env-var for the complete path. Some beats create/store environment variables in build/test.env
. Not sure we do so for x-pack metricbeat, though
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.
IIRC /var/run/docker.sock
does not work on windows too. The compose file only works on Linux right now, add the Docker credendial mounted on the containers will work on any case if the file does not exist will mount nothing. The Oracle test is enabled by an env var disabled by default, so I think it is not an issue to add the Docker folder to the container.
* fix: mount Docker credentials * Apply suggestions from code review * Apply suggestions from code review
Backport some features added to Jenkinsfile to 7.x branch: * Dry run option. * Docker login. * Git config for generator tests. * Filter changes using go list. These are the cherry-picked changes: * fix: login into the docker registry (#17620) * feat: filter changes using go list output (#17397) * fix: disable workaround on macos (#17750) * ci: set git user configuration if it is not set (#17782) * fix: mount Docker credentials (#17798) * Review dependency patterns collection in Jenkins (#18004) Co-authored-by: Ivan Fernandez Calvo <[email protected]> Co-authored-by: Victor Martinez <[email protected]> Co-authored-by: Andrew Kroh <[email protected]>
What does this PR do?
It adds the Docker credentials to the Docker compose.
Why is it important?
We need to login in our registry to grab some Docker images. Removes the
docker pull
introduced on #17620