Revert "Add support for selinux in verbose bind mount specification"

[p12tic]

Reverts containers/podman-compose#911.

The PR still needs tests before being accepted.

[charliemirabile]

I have been using a fork of this project with #911 along with #771 and #763 in production for months. I don't really enjoy maintaining my own fork to get basic features and compatibility with docker-compose, so obviously I want all of the above to be merged and I was extremely excited when #911 was because it meant one fewer patchset I had to carry around downstream. I understand the value of testing, but if you are so insistent about it, perhaps you could make a PR to add missing tests instead of second guessing other maintainers and trying to revert useful changes.

Perhaps if the documentation about what qualifies as adequately tested in your opinion were clearer (e.g. mentioned at all in CONTRIBUTING.md) folks making prs would just be able to get it right the first time instead of trying to be helpful and getting shot down with "needs tests" like literally dozens of PRs in this repo. For what its worth, I saw this pattern in many of the open PRs before I even proposed #911 and so I tried to add some testing, but apparently that wasn't unit testing so it doesn't count. I don't think the notion that every pr needs to have unit tests is documented anywhere other than in the comment you invariably post on every proposed change which just makes the process of contributing to this project unnecessarily frustrating.

I am happy to try and help write tests for my selinux change, but the unit testing code is kind of daunting if you aren't intimately familiar with the framework being used, and it seems a bit haphazard. I didn't see any obvious place to add what I felt should be the small amount of code needed to test my PR without having to add a bunch of infrastructure. It literally just needs to make an example volume description and then verify that z or Z shows up in the resulting arguments, but the functions being changed are hard to test in isolation and there is no existing testing infrastructure that that tests them as far as I am aware.

[p12tic]

Hi @charliemirabile, thanks a lot for response. I will explain my position about why I insist on tests by contributors. Your comment also raised several very good points which I also addressed below.

perhaps you could make a PR to add missing tests instead of second guessing other maintainers and trying to revert useful changes.

This is not a viable long-term strategy. There usually are many contributors, but just a single or several maintainers. If the policy of accepting PRs without tests is established, then few people will write them. Then the maintainer will need to write tests for all PRs. It would be inappropriate to expect this level of effort from a maintainer, especially given that this is an open source project. This will lead to maintainer burnout. This project has already experienced this, as it was unmaintained between summer 2023 and the beginning of this year. Therefore any established policies should prefer to reduce maintainer time expediture as few people can take such role long-term.

folks making prs would just be able to get it right the first time instead of trying to be helpful and getting shot down with "needs tests" like literally dozens of PRs in this repo.

I think it's useful that people do unfinished PRs to gather early feedback. The last thing I would want is for a developer to invert significant amount of time writing tests before making sure that the maintainers agree that the solution itself is acceptable in principle. So I see "looks good, but needs tests" reviews as an advantage. Once the PR is created, if the original contributor does not have interest in writing tests, then someone else can do that and the code changes go in.

I don't think the notion that every pr needs to have unit tests is documented anywhere

That's not true, see https://github.com/containers/podman-compose/blob/v1.1.0/.github/PULL_REQUEST_TEMPLATE.md

However, you're right that the documentation should be improved. The fact that you didn't see it is evidence of that in itself.

functions being changed are hard to test in isolation and there is no existing testing infrastructure that that tests them as far as I am aware.

It's actually relatively easy. If you've written that in a comment on the PR I would have guided you to what's expected.

mount_desc_to_mount_args is called by container_to_args. We have a number of tests for that function already.

An example:

    async def test_gidmaps_extension(self):
        c = create_compose_mock()

        cnt = get_minimal_container()
        cnt['x-podman.gidmaps'] = ['1000:1000:1', '1001:1001:2']

        args = await container_to_args(c, cnt)
        self.assertEqual(
            args,
            [
                "--name=project_name_service_name1",
                "-d",
                "--network=bridge",
                "--network-alias=service_name",
                '--gidmap',
                '1000:1000:1',
                '--gidmap',
                '1001:1001:2',
                "busybox",
            ],
        )

You just modify cnt with the configuration you want to test and then assert what args should look like.

This pattern should be documented somewhere so that people could actually see how easy it is to add tests and then wouldn't be discouraged by my "looks good, but needs tests" reviews.

Thank you for writing the comment with detailed feedback. It raised very clear points on where the development processes of this project should be improved.

[charliemirabile]

Hi @charliemirabile, thanks a lot for response. I will explain my position about why I insist on tests by contributors. Your comment also raised several very good points which I also addressed below.

perhaps you could make a PR to add missing tests instead of second guessing other maintainers and trying to revert useful changes.

This is not a viable long-term strategy. There usually are many contributors, but just a single or several maintainers. If the policy of accepting PRs without tests is established, then few people will write them. Then the maintainer will need to write tests for all PRs. It would be inappropriate to expect this level of effort from a maintainer, especially given that this is an open source project. This will lead to maintainer burnout. This project has already experienced this, as it was unmaintained between summer 2023 and the beginning of this year. Therefore any established policies should prefer to reduce maintainer time expediture as few people can take such role long-term.

This is a fair point. I completely agree that it is not viable or appropriate for maintainers to take on all the burden of writing tests. I still feel that maybe in this specific situation where something gets merged without tests, it might be more productive to try and add the tests than pull the feature back out, but if the PR really was merged in error and the maintainers are on the same page about not wanting it in, a reversion does make sense. That is between you and the other folks maintaining the project. As an outside observer it is just very disheartening to watch your thing go in and then immediately see a new PR to rip it back out.

folks making prs would just be able to get it right the first time instead of trying to be helpful and getting shot down with "needs tests" like literally dozens of PRs in this repo.

I think it's useful that people do unfinished PRs to gather early feedback. The last thing I would want is for a developer to invert significant amount of time writing tests before making sure that the maintainers agree that the solution itself is acceptable in principle. So I see "looks good, but needs tests" reviews as an advantage.

Yes, that is reasonable.

Once the PR is created, if the original contributor does not have interest in writing tests, then someone else can do that and the code changes go in.

Would they file a new PR? I considered doing this for #763 and #771 (though obviously I was going to work on getting #911 acceptable for merge first), but I am not sure about the etiquette there and I wouldn't want to "steal" the credit for the feature just because I was willing to come along and make tests. (even if I include their commit in my branch, it would still show up as "my" PR if I had to file a new one), ant its not like I could push more commits to their branch associated with the existing one.

I don't think the notion that every pr needs to have unit tests is documented anywhere

That's not true, see https://github.com/containers/podman-compose/blob/v1.1.0/.github/PULL_REQUEST_TEMPLATE.md

However, you're right that the documentation should be improved. The fact that you didn't see it is evidence of that in itself.

Ok yes, that actually is pretty clear. I went looking before I wrote my previous comment, but didn't think to check .github. Obviously I did see that when I went to file #911 since it gets filled in to the submission box automatically and you have to delete it to add your message, and while I don't remember exactly my thoughts, I imagine I was like "eh, I have tests" not understanding the distinction that this project makes between the "integration" tests I wrote and the "unit" tests that it is referring to. I was probably also distracted by tracking down the exact citations for the compose spec. To be fair, I am maybe the idiot here for disregarding that relatively clear instruction, but I guess maybe a sentence could be added to explain the two different kinds of tests and that you are referring specifically to adding stuff to the pytests folder.

functions being changed are hard to test in isolation and there is no existing testing infrastructure that that tests them as far as I am aware.

It's actually relatively easy. If you've written that in a comment on the PR I would have guided you to what's expected.

Yeah, it was on my stack to get back to #911 and I would have asked if I couldn't figure it out; just been working on other things. Seeing it get merged reminded me to come check on it which is when I saw this PR.

mount_desc_to_mount_args is called by container_to_args. We have a number of tests for that function already.

An example:

    async def test_gidmaps_extension(self):
        c = create_compose_mock()

        cnt = get_minimal_container()
        cnt['x-podman.gidmaps'] = ['1000:1000:1', '1001:1001:2']

        args = await container_to_args(c, cnt)
        self.assertEqual(
            args,
            [
                "--name=project_name_service_name1",
                "-d",
                "--network=bridge",
                "--network-alias=service_name",
                '--gidmap',
                '1000:1000:1',
                '--gidmap',
                '1001:1001:2',
                "busybox",
            ],
        )

You just modify cnt with the configuration you want to test and then assert what args should look like.

This pattern should be documented somewhere so that people could actually see how easy it is to add tests and then wouldn't be discouraged by my "looks good, but needs tests" reviews.

Yes, this actually is pretty easy. I appreciate the clarification. I think just pointing folks to that file and saying "if you are adding support for new parts of the compose spec you probably need to add one of these tests in this file to check the args you are generating" would go a long way

Thank you for writing the comment with detailed feedback. It raised very clear points on where the development processes of this project should be improved.

Thank you for your helpful reply in spite of the slightly snarky tone I took in my original post here. I appreciate your help and am sorry I took some of my frustration out on you earlier.

Also, is how does this patch look for testing selinux? Happy to make a PR for it, but maybe it would just be easier for you to replace this PR with this.

---
 pytests/test_container_to_args.py | 64 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/pytests/test_container_to_args.py b/pytests/test_container_to_args.py
index f79062d..ccdf757 100644
--- a/pytests/test_container_to_args.py
+++ b/pytests/test_container_to_args.py
@@ -428,3 +428,67 @@ class TestContainerToArgs(unittest.IsolatedAsyncioTestCase):
                 "nvidia-smi",
             ],
         )
+
+    async def test_selinux_volume(self):
+        c = create_compose_mock()
+
+        cnt = get_minimal_container()
+
+        # This is supposed to happen during `_parse_compose_file`
+        # but that is probably getting skipped during testing
+        cnt["_service"] = cnt["service_name"]
+
+        cnt["volumes"] = [
+            {
+                "type": "bind",
+                "source": "./foo",
+                "target": "/mnt",
+                "bind": {
+                    "selinux": "z",
+                },
+            }
+        ]
+
+        args = await container_to_args(c, cnt)
+        self.assertEqual(
+            args,
+            [
+                "--name=project_name_service_name1",
+                "-d",
+                "--mount",
+                "type=bind,source=./foo,destination=/mnt,z",
+                "--network=bridge",
+                "--network-alias=service_name",
+                "busybox",
+            ],
+        )
+
+        cnt["volumes"][0]["bind"]["selinux"] = "Z"
+        args = await container_to_args(c, cnt)
+        self.assertEqual(
+            args,
+            [
+                "--name=project_name_service_name1",
+                "-d",
+                "--mount",
+                "type=bind,source=./foo,destination=/mnt,Z",
+                "--network=bridge",
+                "--network-alias=service_name",
+                "busybox",
+            ],
+        )
+
+        c.prefer_volume_over_mount = True
+        args = await container_to_args(c, cnt)
+        self.assertEqual(
+            args,
+            [
+                "--name=project_name_service_name1",
+                "-d",
+                "-v",
+                "./foo:/mnt:Z",
+                "--network=bridge",
+                "--network-alias=service_name",
+                "busybox",
+            ],
+        )

[p12tic]

As an outside observer it is just very disheartening to watch your thing go in and then immediately see a new PR to rip it back out.

I agree. This was a bit of internal drama between maintainers that you were dragged into with no fault of your own.

Would they file a new PR? I considered doing this for https://github.com/containers/podman-compose/pull/763 and https://github.com/containers/podman-compose/pull/771 (though obviously I was going to work on getting https://github.com/containers/podman-compose/pull/911 acceptable for merge first), but I am not sure about the etiquette there and I wouldn't want to "steal" the credit for the feature just because I was willing to come along and make tests. (even if I include their commit in my branch, it would still show up as "my" PR if I had to file a new one), ant its not like I could push more commits to their branch associated with the existing one.

Well, if someone abandons a PR for a long time then I think it's reasonable to not be surprised if someone else integrates the code and finishes what's missing. I think what's important is to acknowledge the contribution by not changing authorship of the commit and giving credit to the original PR in the new PR. As long that's done, no reasonable person should feel hurt.

Right now I know another person who's going to go PRs in this repo one by one and try adding tests to all of these, so if you're otherwise busy I wouldn't recommend spending your time budget on these. Well, at least for a couple of upcoming weeks, I don't know if the person will deliver.

but didn't think to check .github

Sorry, now I understand why you didn't see it. It is a template added to new PRs, your PR was an older one from before the template existed.

Also, is how does this patch look for testing selinux? Happy to make a PR for it, but maybe it would just be easier for you to replace this PR with this.

The code looks good. I would split the test into three, because it checks three different situation. Maybe parameterized decorator could be used to reduce the code duplication, it's up to you to decide.

Thanks a lot for proposing the test.

[charliemirabile]

The code looks good. I would split the test into three, because it checks three different situation. Maybe parameterized decorator could be used to reduce the code duplication, it's up to you to decide.

Thanks a lot for proposing the test.

How about this? Parameterized is pretty slick, so while I was at it, I added a fourth to round it out (both z and Z with both vol and mount)

 pytests/test_container_to_args.py | 42 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/pytests/test_container_to_args.py b/pytests/test_container_to_args.py
index f79062d..ae6952e 100644
--- a/pytests/test_container_to_args.py
+++ b/pytests/test_container_to_args.py
@@ -3,6 +3,7 @@
 import unittest
 from os import path
 from unittest import mock
+from parameterized import parameterized

 from podman_compose import container_to_args

@@ -428,3 +429,44 @@ class TestContainerToArgs(unittest.IsolatedAsyncioTestCase):
                 "nvidia-smi",
             ],
         )
+
+    @parameterized.expand([
+        (False, "z", ["--mount", "type=bind,source=./foo,destination=/mnt,z"]),
+        (False, "Z", ["--mount", "type=bind,source=./foo,destination=/mnt,Z"]),
+        (True, "z", ["-v", "./foo:/mnt:z"]),
+        (True, "Z", ["-v", "./foo:/mnt:Z"]),
+
+    ])
+    async def test_selinux_volume(self, prefer_volume, selinux_type, expected_additional_args):
+        c = create_compose_mock()
+        c.prefer_volume_over_mount = prefer_volume
+
+        cnt = get_minimal_container()
+
+        # This is supposed to happen during `_parse_compose_file`
+        # but that is probably getting skipped during testing
+        cnt["_service"] = cnt["service_name"]
+
+        cnt["volumes"] = [
+            {
+                "type": "bind",
+                "source": "./foo",
+                "target": "/mnt",
+                "bind": {
+                    "selinux": selinux_type,
+                },
+            }
+        ]
+
+        args = await container_to_args(c, cnt)
+        self.assertEqual(
+            args,
+            [
+                "--name=project_name_service_name1",
+                "-d",
+                *expected_additional_args,
+                "--network=bridge",
+                "--network-alias=service_name",
+                "busybox",
+            ],
+        )

[p12tic]

Looks good

[p12tic]

Feel free to create a PR and I will merge it ASAP and close this one

[charliemirabile]

Alright, just created #946

[p12tic]

Thanks, closing this PR.