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

Fix changes status and return diff information when state=touch and c… #59341

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

Conversation

DSNortsev
Copy link

@DSNortsev DSNortsev commented Jul 20, 2019

SUMMARY

Fixes #50452

  • Add check_model parameter to update_timestamp_for_file function and the file timestamp
    only updates when check_mode is set to False (default option)
  • Changed is set to TRUE if file does not exist or the access_time and modification_time
    option are not set to preserve
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

file

ADDITIONAL INFORMATION

Playbook:

---
- hosts: localhost
  connection: local
  tasks:

  - name: Ensure file for specifications
    file:
      path: /tmp/test
      state: touch

Playbook when access_time and modification_time are preserve:

---
- hosts: localhost
  connection: local
  gather_facts: no
  tasks:
    - name: Ensure file for specifications
      file:
        path: /tmp/test
        state: touch
        access_time: preserve
        modification_time: preserve

Before Fix:

ok: [localhost] => {
    "changed": false,
    "dest": "/tmp/test",
    "gid": 1001,
    "group": "dmitry",
    "invocation"

After Fix if file does not exist:

changed: [localhost] => {
    "changed": true,
    "dest": "/tmp/test",
    "invocation": {
        "module_args":

After Fix if file exists:

changed: [localhost] => {
    "changed": true,
    "dest": "/tmp/test",
    "diff": {
        "after": {
            "atime": 1557243232.714623,
            "mtime": 1557243232.714623,
            "path": "/tmp/test",
            "state": "touch"
        },
        "before": {
            "atime": 1557243223.6023092,
            "mtime": 1557243223.6023092,
            "path": "/tmp/test",
            "state": "file"
        }
    }

After fix if acccess_time and modification_time are set to preserve:

    "changed": false,
    "dest": "/tmp/test",
    "gid": 1001,
    "group": "dmitry",
    "invocation":

@ansibot ansibot added affects_2.9 bug core_review files module needs_triage new_contributor support:community support:core labels Jul 20, 2019
@DSNortsev
Copy link
Author

DSNortsev commented Jul 20, 2019

@bcoca I accidentally closed my previous PR #59219 and was not able to re-open it. I had to create e new one. Also, I am planning to add integration tests.

@DSNortsev
Copy link
Author

DSNortsev commented Jul 21, 2019

@DSNortsev , would be cool to see integration tests
test/integration/targets/file/tasks/main.yml I saw file module tests there

@Andersson007, I added integration tests. Please take a look when you have a chance. Thanks

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Thanks, several cosmetic suggestions.
I can't take a deep review now, maybe later


- name: verify touch updates only mtime when atime is preserved
file: path={{output_dir}}/foo.txt state=touch access_time=preserve

Copy link
Contributor

@Andersson007 Andersson007 Jul 22, 2019

Choose a reason for hiding this comment

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

Suggested change


- name: verify touch updated only atime when mtime is preserved
file: path={{output_dir}}/foo.txt state=touch modification_time=preserve

Copy link
Contributor

@Andersson007 Andersson007 Jul 22, 2019

Choose a reason for hiding this comment

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

Suggested change

@@ -0,0 +1,2 @@
bugfixes:
- file - Fix changed status and return diff information in file module when state=touch and check mode is set https://github.com/ansible/ansible/issues/50452
Copy link
Contributor

@Andersson007 Andersson007 Jul 22, 2019

Choose a reason for hiding this comment

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

Suggested change
- file - Fix changed status and return diff information in file module when state=touch and check mode is set https://github.com/ansible/ansible/issues/50452
- file - Fix changed status and return diff information in file module when state=touch and check mode is set (https://github.com/ansible/ansible/issues/50452)

@@ -0,0 +1,113 @@
# -*- coding: utf-8 -*-
# Copyright:
# (c) 2018 Ansible Project
Copy link
Contributor

@Andersson007 Andersson007 Jul 22, 2019

Choose a reason for hiding this comment

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

Suggested change
# (c) 2018 Ansible Project

@@ -0,0 +1,113 @@
# -*- coding: utf-8 -*-
# Copyright:
Copy link
Contributor

@Andersson007 Andersson007 Jul 22, 2019

Choose a reason for hiding this comment

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

Suggested change
# Copyright:
# Copyright: 2018 Ansible Project

@bcoca bcoca added P3 and removed needs_triage labels Jul 23, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 31, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/units/modules/files/test_file.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

click here for bot help

@ansibot ansibot added ci_verified needs_revision and removed core_review labels Jul 31, 2019
@DSNortsev DSNortsev force-pushed the touch_fix_new branch 2 times, most recently from 8bbe9b9 to 52b3bee Compare Aug 1, 2019
@ansibot ansibot added core_review and removed ci_verified needs_revision labels Aug 1, 2019
@ansibot ansibot added the stale_ci label Aug 9, 2019
Dmitry Nortsev and others added 2 commits Aug 13, 2019
…heck_mode is set

Fixes ansible#50452

* Add check_model parameter to update_timestamp_for_file function and the file timestamp
  only updates when check_mode is set to False (default option)
* Changed is set to TRUE if file does not exist or the access_time and modification_time
  option are not set to preserve
* Add unittest to test touch module in check mode

Signed-off-by: Dmitry Nortsev <dmitry.nortsev@gmail.com>
@ansibot ansibot removed the stale_ci label Aug 13, 2019
@ansibot ansibot added the stale_ci label Aug 29, 2019
@ansibot ansibot added needs_rebase needs_revision and removed core_review labels May 16, 2020
@Akasurde Akasurde requested a review from s-hertel Jun 18, 2020
@Akasurde
Copy link
Member

Akasurde commented Jun 18, 2020

@DSNortsev Could you please rebase the PR using https://docs.ansible.com/ansible/latest/dev_guide/developing_rebasing.html? Thanks.

Copy link
Contributor

@s-hertel s-hertel left a comment

This looks good. lib/ansible/modules/files/file.py should now be lib/ansible/modules/file.py.

# parameters are not set to "preserve"
if prev_state == 'absent':
result['changed'] = True
elif module.params['access_time'] != 'preserve' or module.params['modification_time'] != 'preserve':
Copy link
Contributor

@s-hertel s-hertel Jun 19, 2020

Choose a reason for hiding this comment

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

this is handled by update_timestamp_for_file. You can simplify this and just do an if/else

Suggested change
elif module.params['access_time'] != 'preserve' or module.params['modification_time'] != 'preserve':
else:

if prev_state == 'absent':
result['changed'] = True
elif module.params['access_time'] != 'preserve' or module.params['modification_time'] != 'preserve':
result['changed'] = True
Copy link
Contributor

@s-hertel s-hertel Jun 19, 2020

Choose a reason for hiding this comment

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

Remove this and add it to the conditional below

Suggested change
result['changed'] = True

file_args = module.load_file_common_arguments(module.params)
# Attach diff to result only when the file exists
if update_timestamp_for_file(file_args['path'], mtime, atime, diff, check_mode=True):
result['diff'] = diff
Copy link
Contributor

@s-hertel s-hertel Jun 19, 2020

Choose a reason for hiding this comment

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

Suggested change
result['diff'] = diff
result['diff'] = diff
result['changed'] = True

@Andersson007
Copy link
Contributor

Andersson007 commented Jul 21, 2020

@DSNortsev any updates on this?

@ansibot ansibot added pre_azp and removed stale_ci labels Dec 4, 2020
@ansibot ansibot removed the support:community label Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.9 bug files module needs_rebase needs_revision new_contributor P3 pre_azp support:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants