-
Notifications
You must be signed in to change notification settings - Fork 247
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
MONITOR: disable 'user' config option in case #6642
Conversation
Another thing I would like to address in this PR: currently 'monitor' allows to set arbitrary 'username' in This practically means that option should be Options are: @sumit-bose, @pbrezina, @andreboscatto, what would you say? |
Hi, what about just trying to start as SSSD_USER if the old option was set together with a message in the system logs as well? This should make it easier for users who have to update a large number of systems. bye,
|
9b9f70e
to
862e6b7
Compare
I don't have strong preference, but I am inclined to option 2 to avoid introducing special cases. |
3f7fbdd
to
1e1151c
Compare
Well, on the one hand, having option with type 'string' here is really confusing, and attempt to explain in the man page that "you can only put 'sssd' as a value for this options" doesn't look good. On the other hand, I'd like to simplify things, but introducing a new option while keeping old one feels quite opposite... Nonetheless, since I already had patch according to Sumit's proposal (as far as I understood it) I decide to post it here for review. So, @pbrezina, @sumit-bose, could you please take a look before I update man page? |
I don't have strong preference, so from my perspective, do what you believe is right. But renaming an option is a lot of hustle, we will need to also create leapp actor for it and I don't see strong reason for it. I'd keep it simple. |
Ok, I'll discard current patch and will just add a check that value matches @sumit-bose, do you agree? |
Hi, yes, but maybe it would help to not directly refuse to start if the value is neither bye, |
I don't think it's beneficial to consciously allow misconfiguration and to just replace (even with a warning) config value... |
1e1151c
to
1579fd0
Compare
At the moment, I can't find a simple way to a value of |
Hi, I didn't found a better solution than https://gitlab.freedesktop.org/realmd/realmd/-/commit/98a69ca0 for a similar problem in realmd. HTH bye, |
1579fd0
to
8e5129e
Compare
Thank you. Implemented something similar. |
8e5129e
to
bb6e4d8
Compare
rpm build fails on c8s system with:
Comparing logs with F38, "Discard ... translated; need 80%)" strings looks the same (at least from a quick glance). But resulting set of translation files are very different. F38 -
C8S -
F38:
C8S:
-- empty. So C8S ends up with empty set and fails. So far I don't understand:
|
28547b4
to
49b86d7
Compare
Should be fixed now and ready for review. |
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.
Just a minor change before accepting the PR
--with-sssd-user=root In case SSSD was configured and built --with-sssd-user=root, no other value of 'user' config option (besides default 'root') is supported. Having it documented in the man page in this case only brings confusion.
Only 'root' and SSSD_USER are valid values.
49b86d7
to
466402d
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 for the patch!
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.
Thank you. Ack.
--with-sssd-user=root
In case SSSD was configured and built --with-sssd-user=root, no other value of 'user' config option (besides default 'root') is supported. Having it documented in the man page in this case only brings confusion.