Illumina / GTCtoVCF

Script to convert GTC/BPM files to VCF
Apache License 2.0
42 stars 30 forks source link

Docker container #73

Closed stephenturner closed 2 years ago

stephenturner commented 2 years ago
stephenturner commented 2 years ago

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

jjzieve commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

stephenturner commented 2 years ago

Thanks!