-
Notifications
You must be signed in to change notification settings - Fork 166
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
WIP: fix(config): remove dictsort that breaks sudoers #67
base: master
Are you sure you want to change the base?
Conversation
Best reviewed: commit by commit
Optimal code review plan
|
8802c5b
to
e94e0f2
Compare
@javierbertoli I'll concentrate on the |
in bc62b6e dictsort was applied to the pillar entries. But in the sudoers file *order matters* so, using `dictsort`, breaks it. From man 5 sudoers: > When multiple entries match for a user, they are > applied in order. Where there are multiple matches, the last match > is used (which is not necessarily the most specific match). Removed `dictsort` from the {users,groups,netgroups} specifications. BREAKING CHANGE: sudoers entries' order will change and might break existing configuration. You should check your pillars and rendering.
If I understand correctly:
I understand the need to this PR but it will make the formula application not idempotent again, which is not critical (I think other formulas have this problem too) but not ideal neither. I think one way to solve this problem would be to move from dictionaries to lists for data which needs a specific order but it's a more important change. I'm interested in your opinion on that topic. |
@daks You're right, lists seems a more stable approach. It's a breaking change, anyway, so we'd rather fix it the best way, right? 😄 I'll give it a try. |
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]
Changes related to the build system[chore]
Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]
Changes to the continuous integration configuration[feat]
A new feature[fix]
A bug fix[perf]
A code change that improves performance[refactor]
A code change that neither fixes a bug nor adds a feature[revert]
A change used to revert a previous commit[style]
Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]
Documentation changes[test]
Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE
?YES.
Although it's quite possible that 'nothing will break', existing entries order will change and you might end up with a rendered file that does not match current one.
Related issues and/or pull requests
Describe the changes you're proposing
in bc62b6e dicsort was applied to the pillar entries. But in the
sudoers file order matters so, using
dicsort
, breaks it.From man 5 sudoers:
Removed
dictsort
from the {users,groups,netgroups} specifications.Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README
(e.g.Available states
).pillar.example
.Testing checklist
state_top
).Additional context