Skip to content
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

Bugfix: sing now computes it's parent directory correctly #558

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

KevinFHartmann
Copy link
Contributor

Enhancement: docker client now runs as a song user that has the sing binary in the PATH environment variable

Enhancement: docker client now runs as a song user that has the sing binary in the PATH environment variable
Copy link
Contributor

@rtisma rtisma left a comment

Choose a reason for hiding this comment

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

Please also update the Dockerfile.dev file, and the ./docker/tools/song-client-dev to work. in addition, add this line to the song-client and score-client service definition:
user: "$MY_UID:$MY_GID"

All the scripts in ./docker/tools need to work. For example, in song-client-dev, i had to change the last line from:
run --rm song-client bin/sing $@
to
run --rm song-client ./bin/sing $@

Dockerfile Outdated
@@ -27,6 +27,7 @@ RUN tar zxvf song-client.tar.gz -C /tmp \

# Set working directory for convenience with interactive usage
WORKDIR $SONG_CLIENT_HOME
USER song
Copy link
Contributor

Choose a reason for hiding this comment

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

although i appreciate restricting usage to a user called song, it makes usage of this container a little more difficult. If a directory is mounted, by default its mounted as root and so the user song wont be able to write to it. there are 2 fixes:

  1. run the container with --user "$(id -u):$(id -g)"which overwrites the USER song line you have. this means all files/directories created in the mount path will have the permissions supplied from --user. This basically overrides USER song line

  2. remove the USER song line, so that if a directory/file is created on the mount path by the container, those files/directories will be owned by root, and since the owner of the mounted directory is root, the whole "permission denied" issue is overcome.

Looking at 1 and 2, the best solution i can see is to remove the USER song line (and therefore the groupadd line as well).

@@ -6,4 +6,4 @@ BASH_SCRIPT_DIR=$( dirname "${BASH_SCRIPT}")
DOCKERFILE_NAME=Dockerfile \
docker-compose \
-f ${BASH_SCRIPT_DIR}/../../docker-compose.yml \
run --rm score-client bin/score-client $@
run --rm score-client score-client $@
Copy link
Contributor

Choose a reason for hiding this comment

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

bit of a chicken-and-egg problem here, which is why this command does not work. The docker container for score-client doesnt have the executable on the class path yet. that fix is in this pr: overture-stack/score#215

for consistency sake, change this line back to

run --rm score-client bin/score-client $@

and in a new PR, we can update the version of the score-client/score-server docker images and then use the executable name only

@@ -6,4 +6,4 @@ BASH_SCRIPT_DIR=$( dirname "${BASH_SCRIPT}")
DOCKERFILE_NAME=Dockerfile.dev \
docker-compose \
-f ${BASH_SCRIPT_DIR}/../../docker-compose.yml \
run --rm score-client bin/score-client $@
run --rm score-client score-client $@
Copy link
Contributor

Choose a reason for hiding this comment

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

refer to above comment

Copy link
Contributor

@rtisma rtisma left a comment

Choose a reason for hiding this comment

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

in addition, the line

SONG_CLIENT_CMD := $(DOCKER_COMPOSE_CMD) run --rm -u $(THIS_USER) song-client bin/sing

In the Makefile no longer works. This can be tested with make clean test-manifest

The fix is to drop the bin/, so it would instead be

SONG_CLIENT_CMD := $(DOCKER_COMPOSE_CMD) run --rm -u $(THIS_USER) song-client sing

2) Bugfix: sed command wasn't generating correct analysisId.
@KevinFHartmann KevinFHartmann merged commit ec53a29 into develop Dec 10, 2019
@KevinFHartmann KevinFHartmann deleted the update_clients__score#214 branch December 10, 2019 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants