8000 module_utils: Consolidate set_owner_if_different & set_group_if_different by moreati · Pull Request #75742 · ansible/ansible · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

module_utils: Consolidate set_owner_if_different & set_group_if_different #75742

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
Open

module_utils: Consolidate set_owner_if_different & set_group_if_different #75742

wants to merge 3 commits into from
+103 −55

Conversation

moreati
Copy link
Contributor
@moreati moreati commented Sep 21, 2021 •
edited
Loading
SUMMARY

This merges implementations of AnsibleModule.set_owner_if_different() & AnsibleMo 8000 dule.set_group_if_different() into a single method. The benefits are

  • reduction in code duplication
  • makes possible halving the number of calls to os.lchown() when the copy module sets both owner and group.

There should be no outward change in behavior. The two original methods become thin wrappers around the new one.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

This PR is related to #75741. If both are merged, then the second benefit above can be realised - by replacing calls in copy.chown_recursive().

@ansibot ansibot added affects_2.12 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 21, 2021
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Sep 21, 2021
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 29, 2021
@moreati moreati closed this Nov 12, 2021
@moreati moreati reopened this Nov 12, 2021
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 12, 2021
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 24, 2021
@mattclay mattclay added the P3 Priority 3 - Approved, No Time Limitation label Jul 13, 2022
@moreati moreati force-pushed the chown-if-different branch from 25ee3d0 to 68ab424 Compare July 29, 2022 21:51
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Jul 29, 2022
@moreati
Copy link
Contributor Author
moreati commented Jul 31, 2022

/rebuild

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 31, 2022
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 8, 2022
@moreati moreati force-pushed the chown-if-different branch from 68ab424 to d58ce51 Compare July 8, 2023 00:06
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 8, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 18, 2023
@webknjaz
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link
Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Oct 25, 2024
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Oct 29, 2024
@webknjaz
Copy link
Member

@moreati could you rebase?

@moreati
module_utils: Consolidate set_owner_if_different & set_group_if_diffe…
8904e62 
…rent

This merges implementations of `AnsibleModule.set_owner_if_different()`
& `AnsibleModule.set_group_if_different()` into a single method. The
benefits are
- reduction in code duplication
- makes possible halving the number of calls to `os.lchown()` when the
  copy module sets both owner and group.

There should be no outward change in behavior. The two original methods
become thin wrappers around the new one.

This PR is related to ansible#75741. If both are merged, then the second
benefit above can be realised - by replacing calls in
`copy.chown_recursive()`.

Open questions:

- Should this new method be public (without a leading underscore)?
- Is there a better name?
@moreati moreati force-pushed the chown-if-different branch from d58ce51 to 8904e62 Compare October 29, 2024 15:42
@ansibot ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Oct 29, 2024
@moreati moreati mentioned this pull request Oct 29, 2024
Open
moreati
moreati commented Oct 29, 2024
View reviewed changes
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 12, 2024
s-hertel
s-hertel approved these changes Nov 15, 2024
View reviewed changes
@s-hertel
Copy link
Contributor

Would you mind adding a test? The change looks good to me.

@moreati
Copy link
Contributor Author
moreati commented Nov 17, 2024 •
edited
Loading

Would you mind adding a test? The change looks good to me.

Can do. To clarify, which do you want

  1. just a test/tests for _chown_if_different(), keeping it's current private naming
  2. rename to chown_if_different() (no underscore) and add a test/tests, an extra changelog entry, etc.

or something else?

Edit: clarified option b removes the leading underscore

@moreati
Copy link
Contributor Author
moreati commented Nov 17, 2024

Or (to bikeshed the name) if the function is to be public , then set_owner_and_group_if_different() might fit in better with the existing names.

@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Nov 26, 2024
@moreati moreati force-pushed the chown-if-different branch 3 times, most recently from 3d08885 to 786b378 Compare November 26, 2024 17:10
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Nov 26, 2024
@ansibot
Copy link
Contributor
ansibot commented Nov 26, 2024

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

test/units/module_utils/basic/test_filesystem.py:22:1: E302: expected 2 blank lines, found 1
test/units/module_utils/basic/test_filesystem.py:27:1: E302: expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 26, 2024
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Nov 26, 2024
@moreati
Copy link
Contributor Author
moreati commented Nov 27, 2024

Sorry for the wait @s-hertel, I only spotted your 👍 reaction yesterday.

s-hertel
s-hertel reviewed Nov 27, 2024
View reviewed changes
@moreati @s-hertel
module_utils: Pass through set_owner_and_group_if_different arguments
19bcf69 
Spotted and suggested in code review by s-hertel

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 11, 2024
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-hertel s-hertel s-hertel approved these changes

Assignees
No one assigned
Labels
affects_2.12 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html P3 Priority 3 - Approved, No Time Limitation stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@moreati @webknjaz @s-hertel @ansibot @mattclay
0