Repository: pre-commit/pre-commit
Pull request #1371 (2020-03-19 20:59:04)

Don't use --user when running docker on windows
Previous PR https://github.com/pre-commit/pre-commit/pull/1094 hard code the docker user to `1000:1000` on Windows platform. This is a fragile assumption since not all docker image would have user 1000 configured inside of the container.

Instead, we don't use docker user on Windows to allow it to use the default user defined on the image.

asottile: can you show a failure mode? usually it doesn't matter if there's a user with that uid inside

also it's "docker" ;)
killuazhu: Wow, you reviewed fast... Let me fix some pre-commit issue and also address your comments! @asottile 
killuazhu: > can you show a failure mode? usually it doesn't matter if there's a user with that uid inside

We have a docker image in which we pre-define some git configuration. It's been persisted in `/root/.gitconfig`. When running with a different or non-existed user such as `1000`, he won't be able to retrieve these pre-defined value.
killuazhu: @asottile comments addressed, build has passed! Ready for another look. Thank you!
asottile: > We have a docker image in which we pre-define some git configuration. It's been persisted in `/root/.gitconfig`. When running with a different or non-existed user such as 1000, he won't be able to retrieve these pre-defined value.

I don't think that this fixes that then, since the current user will still be used on POSIX platforms

you could instead put your in-container configuration at `/etc/gitconfig` and then you don't need to worry about the user
killuazhu: > you could instead put your in-container configuration at /etc/gitconfig and then you don't need to worry about the user

You are correct. I'm able to mess around with /etc/gitconfig to persist that config at system level instead of user level.

OTOH, I still think it would be useful to accept the change in this PR. We can avoid using an imaginary user (1000) when running docker on Windows. What do you think?
asottile: I'm torn because the current code "works", this change just makes it different 🤔 

Patch:
diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py
index f449584..4091492 100644
--- a/pre_commit/languages/docker.py
+++ b/pre_commit/languages/docker.py
@@ -76,18 +76,18 @@ def install_environment(
         os.mkdir(directory)
 
 
-def get_docker_user() -> str:  # pragma: win32 no cover
+def get_docker_user() -> Tuple[str, ...]:  # pragma: win32 no cover
     try:
-        return f'{os.getuid()}:{os.getgid()}'
+        return ('-u', f'{os.getuid()}:{os.getgid()}')
     except AttributeError:
-        return '1000:1000'
+        return ()
 
 
 def docker_cmd() -> Tuple[str, ...]:  # pragma: win32 no cover
     return (
         'docker', 'run',
         '--rm',
-        '-u', get_docker_user(),
+        *get_docker_user(),
         # https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container-volumes-from
         # The `Z` option tells Docker to label the content with a private
         # unshared label. Only the current container can use a private volume.


Test patch:
diff --git a/tests/languages/docker_test.py b/tests/languages/docker_test.py
index 171a3f7..b65b223 100644
--- a/tests/languages/docker_test.py
+++ b/tests/languages/docker_test.py
@@ -20,4 +20,4 @@ def test_docker_fallback_user():
         getuid=invalid_attribute,
         getgid=invalid_attribute,
     ):
-        assert docker.get_docker_user() == '1000:1000'
+        assert docker.get_docker_user() == ()