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

Do not add state: absent when a non-existent path is returned in "path" or "dest" #51350

Merged
merged 1 commit into from
Feb 23, 2019

Conversation

samdoran
Copy link
Contributor

SUMMARY

Leave it up to the module to return the state in the results.

I went through all the modules in lib/ansible/modules/files/ and only found one case where the module needed to return state: absent. No other modules return paths that do not exists and will therefore get the correct file type added by AnsibleModule.add_path_info().

Signed-off-by: Sam Doran sdoran@redhat.com

Fixes #35382

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/basic.py
lib/ansible/modules/files/file.py

@samdoran samdoran requested a review from bcoca January 25, 2019 22:35
@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 25, 2019
@bcoca
Copy link
Member

bcoca commented Jan 25, 2019

tempted to say the whole 'add_path_info' should not be automatically called, leave function as it is, but force the call only from specific modules instead of all (pass add_path_info=true? to _json functions?)

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 25, 2019
@dagwieers
Copy link
Contributor

I had no problem doing this myself though. I only needed to know if this would be accepted.
But thanks for taking care of this !

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 25, 2019
@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 Jan 27, 2019
@samdoran
Copy link
Contributor Author

@dag I can't let you have all the fun. 😉

@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 5, 2019
Leave it up to the module to return the state in the results.

I went through all the modules in files/ and only found one case where the module needed to return this. No other modules return paths that do not exists.

Signed-off-by: Sam Doran <sdoran@redhat.com>
@samdoran samdoran force-pushed the issue/35382-state-absent-path branch from 978a488 to f49c3c7 Compare February 13, 2019 20:28
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 13, 2019
@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 21, 2019
Copy link
Contributor

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

LGTM

@dagwieers
Copy link
Contributor

@samdoran Is this ready to be merged ?

@ansibot ansibot removed core_review In order to be merged, this PR must follow the core review workflow. 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 Feb 22, 2019
@ansibot ansibot added shipit This PR is ready to be merged by Core needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Feb 22, 2019
@samdoran
Copy link
Contributor Author

@dag Yes, it's ready to be merged. Just trying to make tests happy though the failures seem unrelated to this PR.

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 23, 2019
@dagwieers dagwieers merged commit cc9c72d into ansible:devel Feb 23, 2019
@dagwieers
Copy link
Contributor

All green. Thanks again !

@samdoran samdoran deleted the issue/35382-state-absent-path branch February 25, 2019 15:35
openstack-gerrit pushed a commit to openstack/openstack-ansible-lxc_hosts that referenced this pull request May 17, 2019
Ansible 2.8 does not return a "state" attribute if the file does not
exist[1], so the task is changed to use stat instead.

[1] ansible/ansible#51350

Change-Id: I166186b6d8abf84317ba98af949d4ed887618912
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request May 17, 2019
* Update openstack-ansible-lxc_hosts from branch 'master'
  - Use stat instead of file module for exists check
    
    Ansible 2.8 does not return a "state" attribute if the file does not
    exist[1], so the task is changed to use stat instead.
    
    [1] ansible/ansible#51350
    
    Change-Id: I166186b6d8abf84317ba98af949d4ed887618912
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

module_utils/basic.py: Adds "state: absent" to result when "path" is returned
5 participants