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

Update file.py's initial_diff() to list existing when state: absent #56353

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@Taoquitok
Copy link

commented May 13, 2019

SUMMARY

Currently when using file: "path=someFolderPath state=absent", if a folder is defined for removing, the diff result only shows the state change, not the files removed. I believe a tweak to initial_diff would make sense in the case of using state=absent to make it more clear what's to be removed

Fixes #55882

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Modules/Files/File.py

ADDITIONAL INFORMATION

When logging ansible output, either to a file, ARA, or else where, having a detailed record of what was changed is useful for in case anything was later found to be incorrectly updated. This is especially useful when bringing non-ansible managed servers into an ansible stack, as they're likely to be snowflakes, with configuration that could break if applying existing roles to it where you would think they overlap.

In my case this issue happened where a default folder that wasn't used any other server got removed from the 1 server it was used on, and lacking a log of what was removed made the recovery process all the more blind.
As I believe it's generally good practice to run a --check run first, adding in a diff of what would be deleted in the situation of the absent statement should protect others from inadvertently removing important files they weren't aware of.

Example playbook tested with stable 2.7 branch
- name: Create example folders
  file:
    path: "{{ item }}"
    state: directory
  with_items:
    - /tmp/example_folder
    - /tmp/example_folder/sub_folder

- name: Create example files
  file:
    path: "{{ item }}"
    state: touch
  with_items:
    - /tmp/example_folder/example_file.tmp
    - /tmp/example_folder/sub_folder/sub_file.tmp

- name: remove example folders and files
  file:
    path: /tmp/example_folder
    state: absent
Before
TASK [remove example folders and files] ********************************************************************************
--- Output before change
+++ after
@@ -1,13 +1,4 @@
 {
     "path": "/tmp/example_folder",
-    "state": "directory"
+    "state": "absent"
 }
Output after change
TASK [remove example folders and files] ********************************************************************************
--- before
+++ after
@@ -1,13 +1,4 @@
 {
     "path": "/tmp/example_folder",
-    "path_content": {
-        "directories": [
-            "/tmp/example_folder/sub_folder"
-        ],
-        "files": [
-            "/tmp/example_folder/example_file.tmp",
-            "/tmp/example_folder/sub_folder/sub_file.tmp"
-        ]
-    },
-    "state": "directory"
+    "state": "absent"
 }

@Taoquitok Taoquitok changed the title Update file.py's initial_diff() to list contents Update file.py's initial_diff() to list contents when state: absent May 13, 2019

@ansibot ansibot added core_review and removed needs_revision labels May 13, 2019

@samdoran

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Please create integration tests and a changelog fragment. See this fragment as an example.

@samdoran samdoran removed the needs_triage label May 21, 2019

@bcoca bcoca changed the title Update file.py's initial_diff() to list contents when state: absent Update file.py's initial_diff() to list existing when state: absent May 21, 2019

@ansibot ansibot added the stale_ci label May 21, 2019

Taoquitok added some commits May 26, 2019

@ansibot ansibot added support:community and removed stale_ci labels May 26, 2019

Taoquitok added some commits May 26, 2019

@ansibot ansibot added needs_revision and removed core_review labels May 26, 2019

@ansibot ansibot added core_review and removed needs_revision labels May 26, 2019

@Taoquitok

This comment has been minimized.

Copy link
Author

commented May 26, 2019

@samdoran
Thanks for checking, I've added the change fragment and a basic check that asserts the new diff property is defined.
How much complexity do you think would be worth adding to confirm the new diff functionality?
Looking at the current tests there's a note to organise the tests based on the different states, so if you'd like a bit more complexity on my tests I could start on the absent state test file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.