Skip to content

fix: mount Docker credentials#17798

Merged
kuisathaverat merged 3 commits intoelastic:masterfrom
kuisathaverat:docker-login
Apr 23, 2020
Merged

fix: mount Docker credentials#17798
kuisathaverat merged 3 commits intoelastic:masterfrom
kuisathaverat:docker-login

Conversation

@kuisathaverat
Copy link
Copy Markdown
Contributor

@kuisathaverat kuisathaverat commented Apr 17, 2020

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

Copy link
Copy Markdown
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@kuisathaverat kuisathaverat marked this pull request as ready for review April 20, 2020 09:27
@kuisathaverat kuisathaverat requested review from a team, sayden and urso April 20, 2020 09:27
volumes:
- ${PWD}/../..:/go/src/github.com/elastic/beats/
- /var/run/docker.sock:/var/run/docker.sock
- ${HOME}/.docker:/root/.docker:ro
Copy link
Copy Markdown

@urso urso Apr 20, 2020

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?

Copy link
Copy Markdown
Contributor Author

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).

Copy link
Copy Markdown

@urso urso Apr 20, 2020

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

Copy link
Copy Markdown
Contributor Author

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.

@kuisathaverat kuisathaverat merged commit be3ec38 into elastic:master Apr 23, 2020
@kuisathaverat kuisathaverat deleted the docker-login branch April 23, 2020 09:11
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Apr 28, 2020
* fix: mount Docker credentials

* Apply suggestions from code review

* Apply suggestions from code review
jsoriano added a commit that referenced this pull request Apr 28, 2020
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 <kuisathaverat@users.noreply.github.com>
Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants