Skip to content

Comments

Add +X capability to linux_acl#62852

Open
leifliddy wants to merge 6 commits intosaltstack:masterfrom
leifliddy:master
Open

Add +X capability to linux_acl#62852
leifliddy wants to merge 6 commits intosaltstack:masterfrom
leifliddy:master

Conversation

@leifliddy
Copy link
Contributor

What does this PR do?

It adds the +X setfacl capability feature.
"The character X stands for the execute permission if the file is a directory or already has execute permission for some user."

What issues does this PR fix or reference?

References: #33921

Previous Behavior

The setfacl characterX was not supported

New Behavior

The setfacl characterX is now supported

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@leifliddy leifliddy requested a review from a team as a code owner October 10, 2022 20:23
@leifliddy leifliddy requested review from dwoz and removed request for a team October 10, 2022 20:23
@leifliddy
Copy link
Contributor Author

Is there any update on this PR?

Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test or X rather than changing the test which uses x.. Seems like by changing this we're removing coverage or x

@leifliddy
Copy link
Contributor Author

Agreed. I wasn't sure quite how to handle that. I'll see what I can do -- but this might be beyond my current abilities. Thanks.

@thecosmicfrog
Copy link

Hi @leifliddy, @dwoz. Has any progress been made on this? Thanks.

@leifliddy
Copy link
Contributor Author

Yes, progress has been made on this. The proposed PR has added support for the setfacl X character
All that's needed now is a functional unit test to be drafted.
TBH, I just don't have the will/desire/time to write + test that out at the moment.
If you look at the current test_present linux_acl.py unit test -- it's nearly 300 lines long.
https://github.com/saltstack/salt/blob/master/tests/unit/states/test_linux_acl.py

So what needs to be done is to create a similar test that tests for the rwX acl

At that topic -- there's likely hundreds of PRs that are stuck in limbo due to the fact that not everyone is familiar with mock object library. The salt team must have known that imposing a hard unit test requirement would lead to this -- so it shouldn't be a total surprise that this has stymied progress on some level. Sorry, but I'm not spending several hours of my time to try and satisfy the unit test requirement when I can just run a cmd.run setfacl function instead to accomplish the same thing.

Feel free the submit your own proposal for that though.

@leifliddy
Copy link
Contributor Author

leifliddy commented Aug 31, 2023

@twangboy Can you please review this change and see if that satisfies the requirement of having a separate test?
Feel free to phrase, reword, modify...etc anything in the PR.

@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:17 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:17 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:17 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:17 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:37 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 02:39 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:05 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:36 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:36 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:36 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:36 — with GitHub Actions Inactive
@leifliddy leifliddy temporarily deployed to ci August 31, 2023 05:36 — with GitHub Actions Inactive
@dwoz
Copy link
Contributor

dwoz commented Jul 12, 2024

@twangboy I think this should be back-ported to 3006.x and then merged forward as it's a long standing bug.

@twangboy
Copy link
Contributor

@leifliddy Could you rebase this PR against the 3006.x branch please?

@dwoz dwoz requested a review from a team as a code owner March 16, 2025 22:09
@twangboy twangboy added test:full Run the full test suite needs-rebase Needs to be rebased, against either the current branch or a different one labels Jul 3, 2025
@twangboy twangboy linked an issue Jul 3, 2025 that may be closed by this pull request
@twangboy
Copy link
Contributor

twangboy commented Dec 9, 2025

Please rebase and address conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Needs to be rebased, against either the current branch or a different one test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow +X in ACL's

6 participants