Change pulp users group from users to pulp #413
Conversation
|
Attached issue: https://pulp.plan.io/issues/7173 |
mdellweg
force-pushed
the
pulp_user_group
branch
from
c83bacd
to
154d457
Compare
molecule/release-static/molecule.yml
Outdated
| verifier: | ||
| name: inspec | ||
| name: ansible | ||
| directory: tests |
There was a problem hiding this comment.
Wow, you managed to fix the inspec tests not running on Python 2.
You need to update this:
https://pulp-installer.readthedocs.io/en/latest/contributing/#molecule-limitations
And I wonder if we even need to installer molecule-inspec. Does it only help with generating the files we need? If so, we should reference that in our contributing.md as a reference.
Is there any reduced functionality when running the tests though?
There was a problem hiding this comment.
Was this a python2 problem? Interesting. But now that you say that, the vm i used to run molecule is using python2. And i must say, i have no idea, how this was supposed to work with name: inspec.
Yes, the verify playbook invests quite some time in installing the inspec on it's own.
There was a problem hiding this comment.
I split the "Do not install molecule-inspec" into a separate commit for easier review.
There was a problem hiding this comment.
@mdellweg I was about to answer your question about how it works, but then I noticed I do not see any commit or PR about this.
There was a problem hiding this comment.
Oh right. That one was adopted by @fao89 in his fixing the CI Marathon.
| @@ -43,6 +43,7 @@ | |||
| shell: '{{ result.stdout.strip() }}' | |||
| home: '{{ pulp_user_home }}' | |||
| system: true | |||
| group: '{{ pulp_group }}' | |||
There was a problem hiding this comment.
So this fixes the issue with new installers. Which is easy.
But what about existing installers? We have to design a solution, even if it means telling users to run chown -R pulp:pulp /var/lib/pulp .
We could recursively chown if we detect that the old version of Pulp was in a specific range. We currently recursively chown for the pulp 2 upgrade scenario.
There was a problem hiding this comment.
I thought about this too, and maybe we "just" pull that chmod out of the pulp2 block. But it might hit performance quite badly on big installations, so we may not want to run it more than once... Can we do something clever with a handler here?
There was a problem hiding this comment.
Seems like the change group recursively is not idempotent. I really do not understand why.
What happens if we do not change the group of existing files? Will pulp stop working properly, or will it "just" be a security concern (in this case a note to the user might be enough)?
There was a problem hiding this comment.
Update: Solved the idempotence issue by specifying follow: false.
There was a problem hiding this comment.
@mdellweg That's a good point. We could run the file module on the dir itself (the task below with recurse=false), and if it is changed, then repeat the task with "recurse=true. We could do it with "when changed", or with a handler (since a handler is normally meant not to define config, but to make an applied config take effect.)
There was a problem hiding this comment.
@mikedep333 as i said, not following symlinks solves the idempotence.
There are static assets linked from the python packages that should not have their group changed anyway.
There was a problem hiding this comment.
So what we have right here works completely, I am just worried about performance. Hence my suggestion to repeat the task recursively if the dir itself needs to be changed.
We could always improve it later if users complain.
mdellweg
force-pushed
the
pulp_user_group
branch
4 times, most recently
from
7f4648c
to
e64d394
Compare
mdellweg
force-pushed
the
pulp_user_group
branch
from
e64d394
to
30c58be
Compare
|
@mikedep333 |
|
Don't think so. |
fixes #7173
https://pulp.plan.io/issues/7173