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

Make file state=absent respect recurse=false for previous state=directory #52656

Open
wants to merge 24 commits into
base: devel
Choose a base branch
from

Conversation

@ewarnke
Copy link

@ewarnke ewarnke commented Feb 20, 2019

SUMMARY

Make file state=absent respect recuse=false for previos state=directory

Fixes #52653
Fixes #22248

ISSUE TYPE
  • Bugfix Pull Request

Sorry about the multiple pulls, not a frequent contributor.

@ewarnke
Copy link
Author

@ewarnke ewarnke commented Feb 20, 2019

Why is shippable using the wrong code branch!?

@ewarnke
Copy link
Author

@ewarnke ewarnke commented Feb 20, 2019

Tests need to be updated to set recurse=true when they want to delete non-empty directory

@ansibot

This comment has been hidden.

Copy link
Member

@dagwieers dagwieers left a comment

LGTM, but this changes behaviour.

lib/ansible/modules/files/file.py Outdated Show resolved Hide resolved
lib/ansible/modules/files/file.py Outdated Show resolved Hide resolved
@dagwieers dagwieers changed the title Make file state=absent respect recuse=false for previos state=directory Make file state=absent respect recurse=false for previous state=directory Feb 20, 2019
@ansibot

This comment has been hidden.

@ewarnke ewarnke force-pushed the ewarnke:patch-2 branch Feb 20, 2019
@ansibot

This comment has been hidden.

@ewarnke
Copy link
Author

@ewarnke ewarnke commented Feb 20, 2019

I get there are behavior changes but the current default behavior is unsafe. I'm open to suggestions on how to better do this without a complete rewrite of the core modules.

@jborean93
Copy link
Contributor

@jborean93 jborean93 commented Feb 25, 2019

CI test failure - https://app.shippable.com/github/ansible/ansible/runs/109917/63/tests

{
    "changed": false,
    "invocation": {
        "module_args": {
            "_diff_peek": null,
            "_original_basename": null,
            "access_time": null,
            "access_time_format": "%Y%m%d%H%M.%S",
            "attributes": null,
            "backup": null,
            "content": null,
            "delimiter": null,
            "directory_mode": null,
            "follow": true,
            "force": false,
            "group": null,
            "mode": null,
            "modification_time": null,
            "modification_time_format": "%Y%m%d%H%M.%S",
            "owner": null,
            "path": "/root/ansible_testing/foobar",
            "recurse": false,
            "regexp": null,
            "remote_src": null,
            "selevel": null,
            "serole": null,
            "setype": null,
            "seuser": null,
            "src": null,
            "state": "absent",
            "unsafe_writes": null
        }
    },
    "msg": "rmtree failed: [Errno 39] Directory not empty: b'/root/ansible_testing/foobar'"
}

This is occurring for all the nodes running the file integration tests.

@dagwieers dagwieers dismissed their stale review Feb 25, 2019

Reported remarks have been fixed

@dagwieers
Copy link
Member

@dagwieers dagwieers commented Feb 25, 2019

@ewarnke I agree, but the warning was intended for others reviewing this PR, not for you :-)

Copy link
Member

@dagwieers dagwieers left a comment

LGTM, however this PR changes the behaviour for existing users, on the other hand it fixes a security-related issue that could cause unintended data-loss. So IMO the impact of the latter trumps the former.

We may want to issue a warning and a deprecation period, rather than an immediate break in behaviour ?

@brontide
Copy link

@brontide brontide commented Feb 25, 2019

Yeah, I've been thinking about abandoning these changes and working on a new pull. What's the thinking on new states, new argument, or new module?

  • States: absent_hard and absent_simple to introduce the new behaviors where absent_hard is the current behavior. That way documentation can be updated over time with the new behavior phased in. I don't like this because it makes a core module feel different.
  • Arguments: recursive_remove could be introduced to control the new behavior where it's yes by default for now and could be changed to no in a later release once people are given time to adjust. We could do a os.rmdir and if it fails, throw a warning about the changed behavior.
  • Module: Part of the problem is creation and destruction of files, links, and directories are subtly different. Maybe we should introduce a link and directory module that can better target these types of functions. That said the downside here is that you could not longer just pass a list of items to file state=absent and have them removed if they are files and directories.

I'm not tied to any one solution and the more I worked on this patch I'm just not happy with the way it's turning out. I'm fairly positive it would introduce major breakage as-is. I'm leaning towards a new argument right now since it seems like a good balance between fixing the bug and breaking existing behaviors while giving a path for people to select how directories are removed.

@brontide
Copy link

@brontide brontide commented Feb 25, 2019

@jborean93 Yeah, I've seen that and even tried chasing a few of the test down to add the flag. The problem is that the current behavior is unsafe and trying to find a good solution that merges cleanly and fixes the unsafe behavior without breaking piles of existing tasks is non-trivial. The fix is easy but people generally don't like breaking changes. - FYI, this is @ewarnke I just forgot to login under my work account.

@samdoran samdoran added the P3 label Feb 26, 2019
@bcoca bcoca removed the affects_2.8 label Feb 26, 2019
ewarnke and others added 23 commits Feb 20, 2019
Make state=absent respect recuse=False for previous state=directory.
Can't unlink directories, need to os.rmdir
Fix integration so that recurse directory delete requires recurse: true
Fix integration test
Update parameter checking, docs, for state=absent recuse=true.
Doc suggestion

Co-Authored-By: ewarnke <32573149+ewarnke@users.noreply.github.com>
Doc language #2

Co-Authored-By: ewarnke <32573149+ewarnke@users.noreply.github.com>
Make state=absent respect recuse=False for previous state=directory.
Can't unlink directories, need to os.rmdir
@ewarnke ewarnke force-pushed the ewarnke:patch-2 branch to 1ad7db4 Mar 20, 2019
Copy link
Contributor

@s-hertel s-hertel left a comment

Getting this into the 2.12 cycle earlier rather than later is probably a good idea.

@@ -0,0 +1,7 @@
---
bugfixes:

This comment has been minimized.

@s-hertel

s-hertel Feb 9, 2021
Contributor

This should probably be a breaking_changes: fragment since it's a very disruptive bugfix.

file:
path: '{{ output_dir }}'
state: absent
recurse: no

This comment has been minimized.

@s-hertel

s-hertel Feb 9, 2021
Contributor

A test deleting an empty directory with recurse: no would be nice.

- Recursively set the specified file attributes on directory contents.
- This applies only to C(state=directory).
- Recursively act on directory contents.
- This applies only to C(state=directory) or C(state=absent).

This comment has been minimized.

@s-hertel

s-hertel Feb 9, 2021
Contributor

We might want to note the version this started applying to C(state=absent), since it looks like the option has been around since 1.1.

This comment has been minimized.

@brontide

brontide Feb 9, 2021

This merge hasn't been updated in a year. It's virtually impossible to fix the unsafe behavior due to the fact that so many tests depend on it.

This comment has been minimized.

@s-hertel

s-hertel Feb 9, 2021
Contributor

Yeah, a lot of things depend on it. The core team discussed an open issue for it on Friday #22248 (comment) without being aware that this PR existed, and consensus was that we'd like to try to move forward with fixing it, as early as possible for the next release cycle so we can get feedback.

This comment has been minimized.

@s-hertel

s-hertel Feb 9, 2021
Contributor

ci_complete can be added to the latest commit to ensure all tests run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment