Skip to content

file: multiple diff support#83445

Open
simonLeary42 wants to merge 8 commits into
ansible:develfrom
simonLeary42:devel3
Open

file: multiple diff support#83445
simonLeary42 wants to merge 8 commits into
ansible:develfrom
simonLeary42:devel3

Conversation

@simonLeary42

@simonLeary42 simonLeary42 commented Jun 13, 2024

Copy link
Copy Markdown
Contributor
SUMMARY

Fixes #83434

Fixes #83435

Update the file module to return a list of diffs when recursively setting file attributes and recursively creating parent directories.

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

In the following example, ansible fails to report that it created multiple direectories, fails to report that it modified file attributes, and then misleads the user into thinking it modified fewer file attributes than it actually did.

example playbook:
Details
- name: play
  hosts: localhost
  gather_facts: false
  tasks:
    - name: create directory with same perms
      ansible.builtin.file:
        path: /tmp/foo/bar
        state: directory
        mode: "0755"
      diff: true

    - name: create directory with different perms
      ansible.builtin.file:
        path: /tmp/fu/bar
        state: directory
        mode: "0770"
      diff: true

    - name: create file with different perms
      ansible.builtin.file:
        path: "{{ item }}"
        state: touch
        mode: "0770"
      diff: false
      loop:
        - /tmp/foo/bar/file1
        - /tmp/foo/bar/file2
        - /tmp/fu/bar/file1
        - /tmp/fu/bar/file2

    - name: recursively modify permissions on directory with same perms
      ansible.builtin.file:
        path: /tmp/foo/bar
        mode: "0755"
        recurse: true
      diff: true

    - name: recursively modify permissions on directory with different perms
      ansible.builtin.file:
        path: /tmp/fu/bar
        mode: "0755"
        recurse: true
      diff: true
playbook output before:
Details
PLAY [play] **********************************************************************************

TASK [create directory with same perms] ******************************************************
--- before
+++ after
@@ -1,3 +1,3 @@
-mode: '0775'
+mode: '0755'
 path: /tmp/foo/bar
-state: absent
+state: directory

changed: [localhost]

TASK [create directory with different perms] *************************************************
--- before
+++ after
@@ -1,3 +1,3 @@
-mode: '0775'
+mode: '0770'
 path: /tmp/fu/bar
-state: absent
+state: directory

changed: [localhost]

TASK [create file with different perms] ******************************************************
changed: [localhost] => (item=/tmp/foo/bar/file1)
changed: [localhost] => (item=/tmp/foo/bar/file2)
changed: [localhost] => (item=/tmp/fu/bar/file1)
changed: [localhost] => (item=/tmp/fu/bar/file2)

TASK [recursively modify permissions on directory with same perms] ***************************
changed: [localhost]

TASK [recursively modify permissions on directory with different perms] **********************
--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/fu/bar

changed: [localhost]

PLAY RECAP ***********************************************************************************
localhost                  : ok=5    changed=5    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
playbook output after:
Details
PLAY [play] **********************************************************************************

TASK [create directory with same perms] ******************************************************
--- before
+++ after
@@ -1,2 +1,2 @@
 path: /tmp/foo
-state: absent
+state: directory

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0775'
+mode: '0755'
 path: /tmp/foo

--- before
+++ after
@@ -1,2 +1,2 @@
 path: /tmp/foo/bar
-state: absent
+state: directory

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0775'
+mode: '0755'
 path: /tmp/foo/bar

changed: [localhost]

TASK [create directory with different perms] *************************************************
--- before
+++ after
@@ -1,2 +1,2 @@
 path: /tmp/fu
-state: absent
+state: directory

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0775'
+mode: '0770'
 path: /tmp/fu

--- before
+++ after
@@ -1,2 +1,2 @@
 path: /tmp/fu/bar
-state: absent
+state: directory

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0775'
+mode: '0770'
 path: /tmp/fu/bar

changed: [localhost]

TASK [create file with different perms] ******************************************************
changed: [localhost] => (item=/tmp/foo/bar/file1)
changed: [localhost] => (item=/tmp/foo/bar/file2)
changed: [localhost] => (item=/tmp/fu/bar/file1)
changed: [localhost] => (item=/tmp/fu/bar/file2)

TASK [recursively modify permissions on directory with same perms] ***************************
--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/foo/bar/file2

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/foo/bar/file1

changed: [localhost]

TASK [recursively modify permissions on directory with different perms] **********************
--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/fu/bar

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/fu/bar/file2

--- before
+++ after
@@ -1,2 +1,2 @@
-mode: '0770'
+mode: '0755'
 path: /tmp/fu/bar/file1

changed: [localhost]

PLAY RECAP ***********************************************************************************
localhost                  : ok=5    changed=5    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. module This issue/PR relates to a module. has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 13, 2024
@ansibot

ansibot commented Jun 13, 2024

Copy link
Copy Markdown
Contributor

The test ansible-test sanity --test mypy --python 3.8 [explain] failed with 7 errors:

lib/ansible/modules/file.py:344:89: assignment: Incompatible default for argument "diff_list" (default has type "None", argument has type "List[Any]")
lib/ansible/modules/file.py:355:32: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:362:32: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:376:40: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:390:81: assignment: Incompatible default for argument "diff_list" (default has type "None", argument has type "List[Any]")
lib/ansible/modules/file.py:400:16: attr-defined: "None" has no attribute "check_mode"
lib/ansible/modules/file.py:411:9: attr-defined: "None" has no attribute "set_fs_attributes_if_different"

The test ansible-test sanity --test mypy --python 3.9 [explain] failed with 7 errors:

lib/ansible/modules/file.py:344:89: assignment: Incompatible default for argument "diff_list" (default has type "None", argument has type "list[Any]")
lib/ansible/modules/file.py:355:32: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:362:32: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:376:40: attr-defined: "None" has no attribute "set_fs_attributes_if_different"
lib/ansible/modules/file.py:390:81: assignment: Incompatible default for argument "diff_list" (default has type "None", argument has type "list[Any]")
lib/ansible/modules/file.py:400:16: attr-defined: "None" has no attribute "check_mode"
lib/ansible/modules/file.py:411:9: attr-defined: "None" has no attribute "set_fs_attributes_if_different"

click here for bot help

Comment thread lib/ansible/modules/file.py Outdated
@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jun 13, 2024
@ansibot

ansibot commented Jun 13, 2024

Copy link
Copy Markdown
Contributor

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

lib/ansible/modules/file.py:344:80: E251: unexpected spaces around keyword / parameter equals
lib/ansible/modules/file.py:344:82: E251: unexpected spaces around keyword / parameter equals
lib/ansible/modules/file.py:390:72: E251: unexpected spaces around keyword / parameter equals
lib/ansible/modules/file.py:390:74: E251: unexpected spaces around keyword / parameter equals

click here for bot help

@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 Jun 13, 2024
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Jun 13, 2024
@simonLeary42

simonLeary42 commented Jun 18, 2024

Copy link
Copy Markdown
Contributor Author

The copy module integration tests caught two bugs in my code that the file module integration tests did not: the first occurred when creating a directory with a relative path, the second occured when trying to create a directory that already exists creating a directory with a path ending with a slash

@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 2, 2024
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 30, 2024

@s-hertel s-hertel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a rather invasive change to the file module. If you can reduce the number of lines you've modified, this would be significantly easier to review.

Comment thread test/integration/targets/file/tasks/directory_as_dest.yml
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 15, 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. labels Nov 15, 2024
Comment thread test/integration/targets/file/tasks/main.yml
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Nov 15, 2024
@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 Nov 15, 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 Dec 2, 2024
@simonLeary42

Copy link
Copy Markdown
Contributor Author

This is a rather invasive change to the file module. If you can reduce the number of lines you've modified, this would be significantly easier to review.

@s-hertel I didn't say anything at the time but I did reduce the number of lines. I reduced it some more just now.

@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 Jun 26, 2025
@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 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue/PR relates to a bug. has_issue module This issue/PR relates to a module. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

file: incomplete/misleading/missing diff when recursively setting attributes file: incomplete diff when creating parent directories

5 participants