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

Docker container #73

Merged
merged 3 commits into from
Apr 15, 2022
Merged

Docker container #73

merged 3 commits into from
Apr 15, 2022

Conversation

stephenturner
Copy link
Contributor

  • Add minimal Dockerfile built on Alpine linux (tiny ~59MB container image)
  • Add documentation on building Dockerfile
  • Add documentation on using Docker container

@stephenturner
Copy link
Contributor Author

Just realized that @jjzieve had the same thoughts years ago in #40

@jjzieve
Copy link
Contributor

jjzieve commented Apr 15, 2022

@stephenturner yes I was gonna mention lol. But your PR seems better (alpine-based, not out-of-date), so I'd be happy to merge this in and close mine. Thanks for the contribution! My only comment on this would be the chmod 777. Is that necessary? Seems insecure.

@stephenturner
Copy link
Contributor Author

Thanks Jake. Disclaimer: not a Docker/security expert by any means, but I think this is okay.

I'm running with docker run -u $(id -u):$(id -g) so that the user inside the container is the same user/group ID as logged in on the host. Otherwise the resulting VCF will get written on the host owned by root, where a regular user can't delete it (or move, or bgzip, etc). The mkdir /data && chmod 777 /data only happens inside the container, not affecting the host machine. This is needed because when the container is run as a regular user, that user can't write to /data inside the container, which is owned by root. Security wise, I think this is actually a little more restrictive, in that if you're running docker without -u $(id -u):$(id -g), the user in the container is root, making everything inside the container effectively 777. This makes only the /data directory 777, with a regular un-privileged user running inside the container.

@jjzieve
Copy link
Contributor

jjzieve commented Apr 15, 2022

Ok, that sounds good then. Let me pull and give it a quick test, then I can merge it.

@jjzieve jjzieve merged commit f809e36 into Illumina:develop Apr 15, 2022
@jjzieve jjzieve mentioned this pull request Apr 15, 2022
@stephenturner
Copy link
Contributor Author

Thanks!

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