If check mode enabled and file missing set changed to true 32676 #33967

pilou- · 2017-12-16T12:18:45Z

SUMMARY

Modify AnsibleModule.set_*_if_different methods: when check_mode is enabled and file is missing, returns True instead of False

Unit tests updated.
Fixes #32676: this allows to avoid exception mentioned in the issue.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

lib/ansible/module_utils/basic.py

ANSIBLE VERSION

ansible 2.5.0 (devel 5fdb39c4d1) last updated 2017/12/15 18:27:27 (GMT +200)

ADDITIONAL INFORMATION

Thanks to @mscherer and @Spredzy for the distributed triplet programming session.

mscherer · 2017-12-16T12:26:00Z

test/units/module_utils/basic/test_set_mode_if_different.py

@@ -89,6 +92,7 @@ def test_mode_unchanged_when_already_0660(am, mock_stats, mocker, mode, check_mo
    am.check_mode = check_mode
    mocker.patch('os.lstat', side_effect=[mock_stats['after'], mock_stats['after'], mock_stats['after']])
    m_lchmod = mocker.patch('os.lchmod', return_value=None, create=True)
+    mocker.patch('os.path.exists', return_value=True)


For consistency, shouldn't you assert that this is called, like os.lchmod ?

Following @Spredzy advice, in order to be consistent with mock of os.lstat, assert not m_path_exists.called was removed :)
We kept it only when return_value of the os.path.exists mock is parametrized.

Adding this assert for every os.path.exists mock doesn't hurt, what do you think ?

pilou- · 2017-12-16T17:32:25Z

CI Errors seems unrelated to these updates:

T=osx/10.11/1

2017-12-16 12:23:35 ERROR: Command "scp -i test/cache/id_rsa -o BatchMode=yes -o ServerAliveCountMax=4 -o ServerAliveInterval=15 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -P 10206 -q -r /tmp/ansible-source-2iEwZC.tgz administrator@parallels-1.testing.ansible.com:/tmp" returned exit status 1.

T=cloud/default/3.6/3

2017-12-16 12:46:57 TASK [azure_rm_virtualmachine_extension : Delete virtual network] **************
2017-12-16 12:48:44 fatal: [localhost]: FAILED! => {"changed": false, "msg": "Error deleting virtual network testVnet - Azure Error: InternalServerError\nMessage: An error occurred."}

mscherer · 2017-12-17T14:45:04Z

shipit

pilou- · 2018-01-03T18:07:27Z

One minor update:

diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index 01192d0b8e..56cd1bd171 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -1474,9 +1474,7 @@ class AnsibleModule(object):
         return changed
 
     def check_file_absent_if_check_mode(self, file_path):
-        if not os.path.exists(file_path) and self.check_mode:
-            return True
-        return False
+        return not os.path.exists(file_path) and self.check_mode
 
     def set_directory_attributes_if_different(self, file_args, changed, diff=None, expand=True):
         return self.set_fs_attributes_if_different(file_args, changed, diff, expand)

abadger · 2018-01-04T16:11:53Z

lib/ansible/module_utils/basic.py

@@ -1450,6 +1473,9 @@ def set_fs_attributes_if_different(self, file_args, changed, diff=None, expand=T
        )
        return changed

+    def check_file_absent_if_check_mode(self, file_path):
+        return not os.path.exists(file_path) and self.check_mode


Tiny optimization: check self.check_mode before checking os.path.exists. That way in the common case we don't have to use a system call to determine if the path exists.

Updated, thanks.

… to True Fixes ansible#32676 Thanks to mscherer and Spredzy for the distributed triplet programming session!

abadger · 2018-01-04T16:48:20Z

@pilou- Looks good. I'm just waiting on the shippable tests to complete.

abadger · 2018-01-05T04:36:58Z

Merged to devel and cherrypicked for 2.4.3rc1

) * basic.py: add mock to os.path.exists * set_*_if_different: if check_mode enabled & file missing: set changed to True Fixes #32676 Thanks to mscherer and Spredzy for the distributed triplet programming session! (cherry picked from commit e9df208)

ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_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. test This PR relates to tests. labels Dec 16, 2017

mscherer reviewed Dec 16, 2017

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 16, 2017

mscherer added ci_verified Changes made in this PR are causing tests to fail. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. labels Dec 17, 2017

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 17, 2017

basic.py: add mock to os.path.exists

01913b8

pilou- force-pushed the if_check_mode_enabled_and_file_missing_set_changed_to_True_32676 branch from 3a942df to d130c09 Compare December 17, 2017 21:28

ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 17, 2017

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 25, 2017

pilou- force-pushed the if_check_mode_enabled_and_file_missing_set_changed_to_True_32676 branch from d130c09 to 6123962 Compare January 3, 2018 18:06

pilou- force-pushed the if_check_mode_enabled_and_file_missing_set_changed_to_True_32676 branch from 6123962 to 0a995d2 Compare January 3, 2018 18:08

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 Jan 3, 2018

pilou- force-pushed the if_check_mode_enabled_and_file_missing_set_changed_to_True_32676 branch from 0a995d2 to e6a22ad Compare January 3, 2018 19:22

pilou- mentioned this pull request Jan 4, 2018

Public Ansible Project Meeting Agenda - January 2018 ansible/community#291

Closed

s-hertel self-requested a review January 4, 2018 15:58

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 4, 2018

abadger reviewed Jan 4, 2018

View reviewed changes

set_*_if_different: if check_mode enabled & file missing: set changed…

4f7cd0d

… to True Fixes ansible#32676 Thanks to mscherer and Spredzy for the distributed triplet programming session!

pilou- force-pushed the if_check_mode_enabled_and_file_missing_set_changed_to_True_32676 branch from e6a22ad to 4f7cd0d Compare January 4, 2018 16:16

s-hertel removed their request for review January 4, 2018 16:40

abadger merged commit e9df208 into ansible:devel Jan 5, 2018

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If check mode enabled and file missing set changed to true 32676 #33967

If check mode enabled and file missing set changed to true 32676 #33967

pilou- commented Dec 16, 2017

mscherer Dec 16, 2017

pilou- Dec 16, 2017

pilou- commented Dec 16, 2017

mscherer commented Dec 17, 2017

pilou- commented Jan 3, 2018 •

edited

abadger Jan 4, 2018

pilou- Jan 4, 2018

abadger commented Jan 4, 2018

abadger commented Jan 5, 2018

If check mode enabled and file missing set changed to true 32676 #33967

If check mode enabled and file missing set changed to true 32676 #33967

Conversation

pilou- commented Dec 16, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

mscherer Dec 16, 2017

Choose a reason for hiding this comment

pilou- Dec 16, 2017

Choose a reason for hiding this comment

pilou- commented Dec 16, 2017

mscherer commented Dec 17, 2017

pilou- commented Jan 3, 2018 • edited

abadger Jan 4, 2018

Choose a reason for hiding this comment

pilou- Jan 4, 2018

Choose a reason for hiding this comment

abadger commented Jan 4, 2018

abadger commented Jan 5, 2018

pilou- commented Jan 3, 2018 •

edited