Syndics honor MoM acl#63428
Merged
Merged
Conversation
dwoz
force-pushed
the
issue/master/62618
branch
5 times, most recently
from
a6bb27a to
43ecda0
Compare
waynew
suggested changes
Jan 9, 2023
waynew
left a comment
waynew
left a comment
Contributor
There was a problem hiding this comment.
Only one problem that I see, and that's in the test suite.
I'm going to check this out and bang on it a bit more. I'm also seeing some failures here: https://github.com/saltstack/salt/actions/runs/3864153908/jobs/6586792092 that are concerning
whytewolf
previously approved these changes
Jan 9, 2023
waynew
suggested changes
Jan 10, 2023
waynew
left a comment
waynew
left a comment
Contributor
There was a problem hiding this comment.
Okay, I opened a PR against your fork and I've also added I think what would make the docstring 👍 - if you want to finish this up I can close mine in favor of this PR, or vice versa and I can go ahead and combine these changes with my PR.
Those smol changes and this is ready to rock! 🎉
Ch3LL
suggested changes
Jan 10, 2023
Contributor
|
Looks like the test failures are possibly related to this change. |
dwoz
force-pushed
the
issue/master/62618
branch
from
7090670 to
8ace01d
Compare
We want to ensure that targeting invalid minions does *not* allow us to run any sort of shenanigans on the minions in question! This ensures that valid commands (i.e. file.touch in this case) cannot inadvertently be sent to minions that we don't have ACL for.
dwoz
force-pushed
the
issue/master/62618
branch
from
ee5ab12 to
9d0ba70
Compare
It's possible that the output of ls is different for some reason, but test -f should always return 0 if the file exists, or 1 otherwise. We want it to be not 0.
whytewolf
approved these changes
Jan 12, 2023
Ch3LL
approved these changes
Jan 12, 2023
9 tasks
garethgreenaway
approved these changes
Jan 12, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This is an alternate approach to #63257
What issues does this PR fix or reference?
Fixes:
#62618
#62577
Previous Behavior
Remove this section if not relevant
New Behavior
Remove this section if not relevant
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.