Skip to content

atomic_move document and warn required abspath, also update callers#83951

Open
bcoca wants to merge 9 commits into
ansible:develfrom
bcoca:atomic_docs
Open

atomic_move document and warn required abspath, also update callers#83951
bcoca wants to merge 9 commits into
ansible:develfrom
bcoca:atomic_docs

Conversation

@bcoca
Copy link
Copy Markdown
Member

@bcoca bcoca commented Sep 17, 2024

related #83950

ISSUE TYPE
  • Docs Pull Request

@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label Sep 17, 2024
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Sep 17, 2024
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Sep 25, 2024
@webknjaz

This comment was marked as off-topic.

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Sep 26, 2024
@ansibot ansibot added module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and 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 Sep 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 Sep 26, 2024
@bcoca bcoca changed the title atomic_move document function atomic_move document and warn required abspath, also update callers Sep 26, 2024
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 26, 2024
Comment thread lib/ansible/module_utils/basic.py Outdated
Comment thread lib/ansible/module_utils/basic.py Outdated
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 10, 2024
@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 Oct 18, 2024
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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
@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 21, 2024
@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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 24, 2025
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 3, 2025
dest_hash = None
src = module.params['src']
dest = module.params['dest']
dest = os.path.abspath(module.params['dest'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of expecting individual modules to convert paths to absolute, we should incorporate the check into the validation for path type module parameters. A deprecation warning can be issued there, and later converted to an error. Once we have controller-side argument processing, it will be able to perform the relative path check.

Having a deprecation warning on atomic_move may still be a good idea, in case there are other direct callers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

agreed the conversion belongs in validation, but I did not want to change it for many other modules that use that as a type but do not require abspath. Also not something I would resolve on controller since paths can differ.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wasn't suggesting the controller would be able to resolve the path, just to error out if it's not absolute.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should avoid calling abspath on these paths, since that will prevent the warning from being shown. Users need to know that the path(s) they have provided may not work as intended. The current working directly isn't always predictable, such as when dropping privileges on macOS, so converting to absolute here just hides the issue.

Comment thread lib/ansible/module_utils/basic.py Outdated
Comment thread lib/ansible/module_utils/basic.py Outdated
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.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 Feb 19, 2025
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 28, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Feb 12, 2026
@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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_pr This PR has not been pushed to for more than one year.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants