-
Notifications
You must be signed in to change notification settings - Fork 80
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 docker user for development #279
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -eo pipefail | ||
|
|
||
| REPO_DIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )/.." && pwd)" | ||
| UNSET_USER=0 | ||
| COMPOSE_FILE="$REPO_DIR/docker-compose.yml" | ||
| COMPOSE_PROJECT="richie" | ||
|
|
||
| # _set_user: set (or unset) default user id used to run docker commands | ||
| # | ||
| # usage: _set_user | ||
| # | ||
| # You can override default user ID (the current host user ID), by defining the | ||
| # USER_ID environment variable. | ||
| # | ||
| # To avoid running docker commands with a custom user, please set the | ||
| # $UNSET_USER environment variable to 1. | ||
| function _set_user() { | ||
|
|
||
| if [ $UNSET_USER -eq 1 ]; then | ||
| USER_ID="" | ||
| return | ||
| fi | ||
|
|
||
| # USER_ID = USER_ID or `id -u` if USER_ID is not set | ||
| USER_ID=${USER_ID:-$(id -u)} | ||
|
|
||
| echo "🙋(user) ID: ${USER_ID}" | ||
| } | ||
|
|
||
| # docker_compose: wrap docker-compose command | ||
| # | ||
| # usage: docker_compose [options] [ARGS...] | ||
| # | ||
| # options: docker-compose command options | ||
| # ARGS : docker-compose command arguments | ||
| function _docker_compose() { | ||
|
|
||
| echo "🐳(compose) project: '${COMPOSE_PROJECT}' file: '${COMPOSE_FILE}'" | ||
| docker-compose \ | ||
| -p "${COMPOSE_PROJECT}" \ | ||
| -f "${COMPOSE_FILE}" \ | ||
| --project-directory "${REPO_DIR}" \ | ||
| "$@" | ||
| } | ||
|
|
||
| # _dc_run: wrap docker-compose run command | ||
| # | ||
| # usage: _dc_run [options] [ARGS...] | ||
| # | ||
| # options: docker-compose run command options | ||
| # ARGS : docker-compose run command arguments | ||
| function _dc_run() { | ||
| _set_user | ||
|
|
||
| echo "🐳(compose) run command: '$@'" | ||
|
|
||
| user_args="--user=$USER_ID" | ||
| if [ -z $USER_ID ]; then | ||
| user_args="" | ||
| fi | ||
|
|
||
| _docker_compose run --rm $user_args "$@" | ||
| } | ||
|
|
||
| # _dc_exec: wrap docker-compose exec command | ||
| # | ||
| # usage: _dc_exec [options] [ARGS...] | ||
| # | ||
| # options: docker-compose exec command options | ||
| # ARGS : docker-compose exec command arguments | ||
| function _dc_exec() { | ||
| _set_user | ||
|
|
||
| echo "🐳(compose) exec command: '$@'" | ||
|
|
||
| user_args="--user=$USER_ID" | ||
| if [ -z $USER_ID ]; then | ||
| user_args="" | ||
| fi | ||
|
|
||
| _docker_compose exec $user_args "$@" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| source "$(dirname "${BASH_SOURCE[0]}")/_config.sh" | ||
|
|
||
| _dc_exec "$@" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| docker-compose \ | ||
| -p richie-test \ | ||
| -f docker/compose/test/docker-compose.yml \ | ||
| --project-directory . \ | ||
| run --rm --no-deps app pylint "$@" | ||
| source "$(dirname "${BASH_SOURCE[0]}")/_config.sh" | ||
|
|
||
| COMPOSE_FILE="$REPO_DIR/docker/compose/test/docker-compose.yml" | ||
| COMPOSE_PROJECT="richie-test" | ||
|
|
||
| _dc_run --no-deps app pylint "$@" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| docker-compose \ | ||
| -p richie-test \ | ||
| -f docker/compose/test/docker-compose.yml \ | ||
| --project-directory . \ | ||
| run --rm app pytest "$@" | ||
| source "$(dirname "${BASH_SOURCE[0]}")/_config.sh" | ||
|
|
||
| COMPOSE_FILE="$REPO_DIR/docker/compose/test/docker-compose.yml" | ||
| COMPOSE_PROJECT="richie-test" | ||
|
|
||
| _dc_run app pytest "$@" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| source "$(dirname "${BASH_SOURCE[0]}")/_config.sh" | ||
|
|
||
| _dc_run "$@" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Isn't it what we want for the CI and production as well (as opposed to running as root)?
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 surely do not want to (shouldn't?) run with the root user in most cases, you are right.
Remember that only the dev flavor of Richie's image runs with the root user (we use the
UID=10000in "production"), and, we use the entrypoint trick to allow running commands in Richie containers whatever the container user will be. This allows us to map local user ID with container user ID and mount local volumes without falling into file permission issues.In the CI, we do not want to mount volumes as we want to test our newly built docker image. Hence, if we run as a non-root user, we will fall into file permission issues in the container itself (tests, coverage, etc. will try to create files in
/app, a directory that belongs to therootuser). One way to solve this would be to change the ownership of the/appdirectory fromrootto a lambda user. But I think it's not a good practice: I prefer having a running user that has no write permissions at all in the container.To make a long story short, I think running tests with the root user in the CI is a lesser evil than allowing the container running user to write in the application's directory.
WDYT?
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 would argue that you should run the CI in conditions as close as possible to the production.
If you run as root and your app writes inside the container to places where it won't be allowed to write in production, then your tests could miss something big...
So maybe we should make the same mounts as in production and run as an unpriviledged user to precisely check that our container works in "readonly" mode.
On more way to look at it is that this seemingly small difference is the whole reason why we have to modify images in https://www.github.com/openshift-docker. The original images from which we build in this repo would pass your CI with a root user but would fail to work in production.
This being said, maybe we can decide that what we are testing here is the code and that the images will be tested through infrastructure tests.
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.
True.
Also true. Moreover the dev flavor of the docker image that is used for tests has many additional dependencies installed; potentially changing the app behaviour.
Good idea. But, I was talking about mounting the code as volume for tests. This is not what we want.
That would be an interesting approach.
Conclusion: we need to switch to a random user also for tests (read-only mode); I hope we will be able to configure our (test|lint)-suite to fit with this requirement. I'll declare a new issue for this.