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

add path to return val of file touch #55904

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@mlda065
Copy link
Contributor

commented Apr 30, 2019

SUMMARY

Fixes #55898

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

file module

ADDITIONAL INFORMATION

Here is a playbook which fails without this patch, and passes with it.

---
- hosts: localhost
  tasks:


     - name: touch file
       file:
          state: touch
          path: file.txt
       register: touch
       failed_when: touch.path != 'file.txt'

     - name: file check
       file:
          state: file
          path: file.txt
       register: file
       failed_when: file.path != 'file.txt'

     - name: remove file
       file:
          state: absent
          path: file.txt
       register: absent
       failed_when: absent.path != 'file.txt'


     - name: directory check
       file:
          state: directory
          path: dir
       register: dir
       failed_when: dir.path != 'dir'
@s-hertel
Copy link
Contributor

left a comment

It seems like it would be good to document the return values instead since the path is already available in the output via dest. Here's an example of what that looks like:
https://docs.ansible.com/ansible/latest/modules/copy_module.html#return-values
https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/files/copy.py#L197-L258

@ansibot ansibot added needs_revision and removed core_review labels Apr 30, 2019

@mlda065

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Adding an explanation of return values to the documentation would be good too.
I can do that.

I think that the code itself does need to change. Currently state: touch returns ret.dest, but state: file returns ret.path. That inconsistency is a bad thing.

I don't think there's any substantial drawback to modifying both. It's not even that inefficient to return both path and dest, since python will just make two pointers to the same malloced string.

@mlda065

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Looking at the code, it is a bit inconsistent.

state: hard returns path if no change is made and path is a directory. Otherwise it returns dest.

What should the return behavior be for link and hard?

@mlda065

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Also, state: link returns dest on success, but path on failure. What should it be?

@mlda065

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Also, when you have state: touch, the return value is state: file. That's really confusing. What is it supposed to be?

@ansibot ansibot removed the small_patch label May 1, 2019

@mlda065

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I was a bit unclear on what to put for some of the details of the return values.

Here is a playbook which passes (with this patch). I wrote the documentation based on this behavior:

---
- hosts: localhost
  tasks:

     - name: touch file
       file:
          state: touch
          path: file.txt
       register: touch
       failed_when: >
          (touch.path != 'file.txt') or
          (touch.state != 'file') or
          ((touch.diff is not defined) and touch.changed)

     - name: file check
       file:
          state: file
          path: file.txt
       register: file
       failed_when: >
          (file.path != 'file.txt') or
          (file.state != 'file') or
          ((file.diff is not defined) and file.changed)

     - name: create soft link
       file:
          dest: link.txt
          src: file.txt
          state: link
       register: link
       failed_when: >
         (link.dest != 'link.txt') or
         (link.state != 'link') or
         ((link.diff is not defined) and link.changed)

     - name: touch link
       file:
         state: touch
         path: link.txt
       register: link_touch
       failed_when: link_touch.state != 'link'

       # because the next task will fail with a relative path
     - name: create file from absolute path
       file:
         path: /tmp/file.txt
         state: touch

     - name: create hard link
       file:
          dest: hard.txt
          src: /tmp/file.txt
          state: hard
       register: hard
       failed_when: >
         (hard.dest != 'hard.txt') or
         (hard.state != 'hard') or
         ((hard.diff is not defined) and hard.changed)

     - name: touch hard
       file:
         state: touch
         path: hard.path
       register: hard_touch
       failed_when: hard_touch.state != 'file'

     - name: remove file
       file:
          state: absent
          path: file.txt
       register: absent
       failed_when: >
          (absent.path != 'file.txt') or
          (absent.path is not defined) or
          (absent.state != 'absent') or
          ((absent.diff is not defined) and absent.changed)

     - name: directory check
       file:
          state: directory
          path: dir
       register: dir_check
       failed_when: >
         (dir_check.path != 'dir') or
         (dir_check.state != 'directory') or
         ((dir_check.diff is not defined) and dir_check.changed)

     - name: touch directory
       file:
         state: touch
         path: dir
       register: dir_touch
       failed_when: dir_touch.state != 'directory'
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.