-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the pidfile accessible by everyone #3252
base: main
Are you sure you want to change the base?
Conversation
To discuss here which of the added |
22ff571
to
f5768f3
Compare
83819a0
to
d2dafc9
Compare
d2dafc9
to
acb4820
Compare
d45af53
to
0dc6e94
Compare
0dc6e94
to
124aa95
Compare
@happz one question, in the related issue with the patch it's used |
Is your change here causing a regression? Or is your change here just making no difference WRT #2877? |
Updated ! |
73bf14b
to
110b9b1
Compare
Summary from the chat discussion:
|
fcf637b
to
b3a7b33
Compare
b3a7b33
to
0487765
Compare
tests/provision/facts/test-guest.sh
Outdated
@@ -40,7 +42,7 @@ rlJournalStart | |||
fi | |||
|
|||
elif [ "$PROVISION_HOW" = "container" ]; then | |||
provision_options="--image fedora:39" | |||
provision_options="--image $TEST_IMAGE_PREFIX/fedora/39/unprivileged:latest" | |||
bfu_provision_options="$provision_options --user=nobody" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--user
would need to change as well, to fedora
, and the main image shouldn't be the unprivileged one. Together, I think something like this should be the outcome:
provision_options="--image $TEST_IMAGE_PREFIX/fedora/39:latest"
bfu_provision_options="--image $TEST_IMAGE_PREFIX/fedora/39/unprivileged:latest --user=fedora"
6c55455
to
88a81e5
Compare
It seems now there's an issue when setting the ACLs, the user doesn't have permissions
Also I see some extra error about an rsync command along with the following message |
88a81e5
to
8b71eec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
I see now the failed tests are
Sorry I don't see how is this related with the patch... 😕 @happz @psss could you shed some light here? |
Hmmm, these seem to be caused by GitHub randomly refusing to serve the raw content:
Sounds like they enabled some rate limiting, or we hit the limit: Any suggestions? |
8b71eec
to
ea92b5a
Compare
I see this test case is also failing:
I think the same approach for the pidfile can be used?, something like: sudo = 'sudo' if not self.facts.is_superuser else ''
...
{sudo} setfacl -d -m o:rX {workdir_root}
""")) |
Creates the pidfile at an accessible location for every user, this way the manage pidfile warning is avoided. Signed-off-by: mcasquer <[email protected]>
when it doesn't exist. Signed-off-by: mcasquer <[email protected]>
Also formats the pid directory creation command. Signed-off-by: mcasquer <[email protected]>
This way we avoid TestingFarm to set the old values. Signed-off-by: mcasquer <[email protected]>
in the pidfile creation. Also includes the super().setup() call to the setup GuestSsh function. Signed-off-by: mcasquer <[email protected]>
every time the user has no privileges Signed-off-by: mcasquer <[email protected]>
and fedora user Signed-off-by: mcasquer <[email protected]>
ea92b5a
to
0f1a806
Compare
Yes, I think if not self.facts.is_superuser and self.become Meaning that we should probably use it here always. @happz, any thoughts? @carlosrodfern, as this |
Anyway, this is too late for 1.39. Let's merge this as one of the first changes in 1.40 in order to give it some testing. |
For what I can understand, when the I may be missing something too... |
Creates the pidfile at an accessible location
for every user, this way the manage pidfile
warning is avoided.
Pull Request Checklist