GeezerVison

It's my first attempt to write a python patch ever. I apologize if I did it wrong.

Build failing on coverage, I am afraid it is beyond my knowledge to fix this :/.

Now I am facing a coverage error: https://asottile.visualstudio.com/asottile/_build/results?buildId=3831&view=logs&j=291e3f77-befc-520d-9779-f5b46c027190&t=037f03e6-93d9-56f0-123e-44a4e724aa8c&l=73

I am clueless on how to fix it:

coverage report
Name                             Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------
pre_commit/languages/docker.py      64      3      8      1    92%   89->90, 90-92

I spent some time trying to understand how to write a test to make the build pass. I just don't understand how to get it. Too much to learn in one go. May I request some help (again)?

So far I understand that it should probably take the for of a block like that in tests/languages/docker_test.py.

Then I assume we have to mock the content of output to test the behaviour of the for ... if ... if.

Then I got lost in the pytest documentation.

def test_docker_system_info():
    <here be dragons>:
        assert  <here be dragons>
``

@themr0c you can use a built-in fixture called monkeypatch (for this, just add monkeypatch as an argument of your test method) to mock subprocess.check_output with a fake function that returns a sample output of podman (multiline).

The simple one case test would be

_ROOTLESS_DOCKER_OUTPUT = """
some garbage
   rootless: true
      even more garbage
"""

def test_rootless_docker(monkeypatch):
    """Verify that only rootless docker/podman doesn't add args."""
    monkeypatch.setattr('subprocess.check_output', lambda cmd, text: _ROOTLESS_DOCKER_OUTPUT)
    assert docker.get_docker_user() == ()

And going further this would create tests for 4 test cases (rootless docker, rootless podman, non-rootless docker and non-rootless podman):

import os

import pytest

_CURRENT_UID = os.getuid()
_CURRENT_GID = os.getgid()


@pytest.mark.parametrize(
    ('docker_sys_info', 'expected_args'),
    (
        pytest.param(
            """
            some garbage
               rootless: true
                  even more garbage
            """,
            (),
            id='rootless podman',
        ),
        pytest.param(
            """
            some garbage
               rootless: false
                  even more garbage
            """,
            (_CURRENT_UID, _CURRENT_GID),
            id='non-rootless podman',
        ),
        pytest.param(
            """
            some garbage
               rootless
                  even more garbage
            """,
            (),
            id='rootless docker',
        ),
        pytest.param(
            """
            some garbage
               nothing good
                  even more garbage
            """,
            (_CURRENT_UID, _CURRENT_GID),
            id='non-rootless docker',
        ),
    ),
)
def test_rootless_docker(docker_sys_info, expected_args, monkeypatch):
    """Verify that only rootless docker/podman doesn't add args."""
    monkeypatch.setattr('subprocess.check_output', lambda cmd, text: docker_sys_info)
    assert docker.get_docker_user() == expected_args

before going further please address my comments here

in particular:

• this needs to not run every time
• please don't use subprocess.check_output, there's a helper (already used throughout the rest of the module(s))

additionally:

• please don't use monkeypatch, follow the style of the rest of the codebase (unittest.mock.patch.object)
• if you're going to write tests targetting the docker output, please make them more like real ones (they would not be indented, they would not say "garbage", etc.)

I am completely illiterate in writing python code, it seems I chose a task too complex for me as first attempt to contribute :/.

• Is there some example somewhere where I could learn to use @functools.lru_cache(maxsize=1) by copy-pasting?
• What does the replacement helper for subprocess.check_output look like?
• I have read the other tests ans seen some unittest.mock.patch.object. I still don't understand how to write a proper test.

@themr0c you literally can just import functools and paste @functools.lru_cache(maxsize=1) right before def get_docker_user(): ... (in the previous line). LRU cache only works during the same runtime session. So if you run the program (pre-commit) again, it'll not hit the cache and execute the function. I suppose Anthony wants the cached result to be preserved across pre-commit runs which is more complicated. Also, I'd factor out the logic of identifying the rootless install into a separate helper function and apply caching just to that function.

@themr0c you literally can just import functools and paste @functools.lru_cache(maxsize=1) right before def get_docker_user(): ... (in the previous line). LRU cache only works during the same runtime session. So if you run the program (pre-commit) again, it'll not hit the cache and execute the function. I suppose Anthony wants the cached result to be preserved across pre-commit runs which is more complicated. Also, I'd factor out the logic of identifying the rootless install into a separate helper function and apply caching just to that function.

in-process cache is fine -- there's currently no precedent for across-process caching in pre-commit, though I plan to do that eventually for virtualenv invalidation

I am completely illiterate in writing python code, it seems I chose a task too complex for me as first attempt to contribute :/.

you're doing great! we'll help you through it :)

* Is there some example somewhere where I could learn to use `@functools.lru_cache(maxsize=1)` by copy-pasting?

in this case it would just be importing functools and adding that decorator to the function that's being modified here

* What does the replacement helper for `subprocess.check_output` look like?

the helper is cmd_output and you can use it like cmd_output('docker', '--version') (it returns (retcode, stdout, stderr))

* I have read the other tests ans seen  some `unittest.mock.patch.object`. I still don't understand how to write a proper test.

taking the test above, you'd use

with mock.patch.object(docker, 'cmd_output', return_value=(0, ..., '')):
    ...

instead of the monkeypatch of check_output

Side note: We have a videocall scheduled with @themr0c for early next week (this week it didn't work out) and we'll go trough the requested changes together.

@themr0c I think this PR's title/description should be updated.

let me know if you'd like me to finish this one, I've finally gotten around to setting up podman to reproduce this

Depends on @themr0c. I am available to meet again and address the review comments, but if they prefer you to handle it, I don't mind.

@asottile @hroncok If you manage to finish this one, I would be very happy. I am not sure I would find time during the next coming month.

I may be able to take a look at this as I use CentOS and have both docker and podman installed. Are the review comments still current?

looks like --userns=keep-id works slightly better on podman, no idea if this works well for docker though 🤔 https://stackoverflow.com/a/63767259/812183

The user option is not necessary, podman does the correct mapping.

$ ls -ltr toto
ls: cannot access 'toto': No such file or directory
$ podman  run --rm -ti   -v $(pwd):/docs:Z antora/antora touch /docs/toto
/docs/tox.ini-rw-rw-r--    1 root     root          7966 Sep 14 11:22 /docs/tox.ini
$ ls -ltr toto
-rw-r--r--. 1 ffloreth ffloreth 0 14 sep 14:36 toto

On the contrary, specifying user and userns can lead to trouble:

$ podman  run --rm -ti   --userns=keep-id --user $(id -u):$(id -g)   -v $(pwd):/docs:Z antora/antora ls -ltr /docs/tox.ini
Error: requested user's UID 105971 is too large for the rootless user namespace

One of the main advantages of podman is to get rid of the -u option that you need when you run docker if you don't want to see your workspaces filled by files owned by root... So why insist on keeping some unnecessary complexity?

the default mapping gives too much permission and can create undeletable files outside:

$ podman run --rm -ti -v $PWD:/z:rw ubuntu:focal bash -c 'mkdir -p /z/1/2/3 && chown -R nobody /z/1'
$ rm -rf 1
rm: cannot remove '1/2/3': Permission denied

Now I undertand better.

looks like --userns=keep-id works slightly better on podman, no idea if this works well for docker though 🤔 stackoverflow.com/a/63767259/812183

Came here from #2173 and would be interested in taking this over.

Would it be sufficient to return ('-u', f'{os.getuid()}:{os.getgid()}', '--userns=keep-id') in get_docker_user() if (a) docker is an alias for podman and (b) rootless mode is detected?

@PhilipTrauner please take it over. I am happy if this pull request gets closed, even unresolved. I apologize for having started something that I am not able to finish.

Closing, as this pull request implements the feature: kubernetes/test-infra#25841