Skip to content
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

[3006.x] Fix user.present state when group is unset #64530

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Jun 22, 2023

What does this PR do?

If the group arg is unset when using user.present it should not edit the groups of the user. This is documented behavior. This was changed in this PR #62503 to ensure optional_groups are fixed. This PR ensures we keep the optional_groups working and ensure the documented behavior when group args is unset.

What issues does this PR fix or reference?

Fixes: #64211

Previous Behavior

When group was unset when using user.present it would edit the group list if it didn't mirror what groups where currently in the system, resulting in salt returning a changes directory each time.

New Behavior

When group is unset when using user.present` it does not edit the groups the user is a part of.

@Ch3LL Ch3LL requested a review from a team as a code owner June 22, 2023 18:24
@Ch3LL Ch3LL requested review from dwoz and removed request for a team June 22, 2023 18:24
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix user.present state when group is unset [3006.x] Fix user.present state when group is unset Jun 22, 2023
@Ch3LL Ch3LL requested review from nicholasmhughes and removed request for dwoz June 22, 2023 18:24
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:39 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:39 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:39 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:40 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:56 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 00:59 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:42 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:42 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:43 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:43 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:43 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 01:43 — with GitHub Actions Inactive
@@ -189,6 +189,8 @@ def test_present_uid_gid_change():
"user.chgid": Mock(),
"file.group_to_gid": mock_group_to_gid,
"file.gid_to_group": mock_gid_to_group,
"group.info": MagicMock(return_value=after),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicholasmhughes I'm cautious about this change to this test, since group.info and user.chgroups was not called previously. But I think with my fix it should be getting called to ensure we are changing the user foo's group assignment to othergroup. Wanted to get your thoughts since you last changed this test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems reasonable to me. 👍

@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:07 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:07 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:07 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:07 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:26 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 18:26 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:12 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:12 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:12 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:12 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:13 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci June 23, 2023 19:13 — with GitHub Actions Inactive
@@ -189,6 +189,8 @@ def test_present_uid_gid_change():
"user.chgid": Mock(),
"file.group_to_gid": mock_group_to_gid,
"file.gid_to_group": mock_gid_to_group,
"group.info": MagicMock(return_value=after),
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems reasonable to me. 👍

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

Successfully merging this pull request may close these issues.

3 participants