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

Allow to users to touch file with Unix date of birth #49310

Merged
merged 2 commits into from
Sep 17, 2018
Merged

Allow to users to touch file with Unix date of birth #49310

merged 2 commits into from
Sep 17, 2018

Conversation

importepeu
Copy link
Contributor

@importepeu importepeu commented Aug 24, 2018

What does this PR do?

Change file.touch management to allow users to specify atime or mtime values of '0' to touch file with Unix epoch time 1970-01-01 01:00:00.000000000.

What issues does this PR fix or reference?

None

Previous Behavior

Setting atime or mtime with "0" touched the file with current timestamp instead of Unix date of birth.

New Behavior

Now users can set atime or mtime values to '0' to get Unix date of birth, set them to other positive values or unset them to touch file with current time.

Tests written?

No but I've tested the code like this :

$ salt 'ulgcicpi50' file.touch name=/etc/salt/grains atime="\"0\""
ulgcicpi50:
    True
$ stat /etc/salt/grains
  File: '/etc/salt/grains'
  Size: 848             Blocks: 8          IO Block: 4096   regular file
Device: fc00h/64512d    Inode: 14893       Links: 1
Access: (0640/-rw-r-----)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 1970-01-01 01:00:00.000000000 +0100
Modify: 2018-08-24 15:36:46.491617000 +0200
Change: 2018-08-24 15:36:46.488108473 +0200
 Birth: -
$ salt 'ulgcicpi50' file.touch name=/etc/salt/grains mtime="\"0\""
ulgcicpi50:
    True
$ stat /etc/salt/grains
  File: '/etc/salt/grains'
  Size: 848             Blocks: 8          IO Block: 4096   regular file
Device: fc00h/64512d    Inode: 14893       Links: 1
Access: (0640/-rw-r-----)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2018-08-24 15:37:06.700995000 +0200
Modify: 1970-01-01 01:00:00.000000000 +0100
Change: 2018-08-24 15:37:06.696237885 +0200
 Birth: -
$ salt 'ulgcicpi50' file.touch name=/etc/salt/grains mtime="\"0\"" atime="\"0\""
ulgcicpi50:
    True
$ stat /etc/salt/grains
  File: '/etc/salt/grains'
  Size: 848             Blocks: 8          IO Block: 4096   regular file
Device: fc00h/64512d    Inode: 14893       Links: 1
Access: (0640/-rw-r-----)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 1970-01-01 01:00:00.000000000 +0100
Modify: 1970-01-01 01:00:00.000000000 +0100
Change: 2018-08-24 15:37:28.344376504 +0200
 Birth: -
$ salt 'ulgcicpi50' file.touch name=/etc/salt/grains
ulgcicpi50:
    True
$ stat /etc/salt/grains
  File: '/etc/salt/grains'
  Size: 848             Blocks: 8          IO Block: 4096   regular file
Device: fc00h/64512d    Inode: 14893       Links: 1
Access: (0640/-rw-r-----)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2018-08-24 15:37:42.732468628 +0200
Modify: 2018-08-24 15:37:42.732468628 +0200
Change: 2018-08-24 15:37:42.732468628 +0200
 Birth: -

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Change file.touch management to allow user to specify atime or mtime values of '0' to touch file with Unix epoch time 1970-01-01 01:00:00.000000000. Only positive value for atime and mtime was allowed.
@importepeu importepeu changed the title Update file.py Allow to users to touch file with Unix date of birth Aug 24, 2018
@cachedout
Copy link
Contributor

cachedout commented Aug 24, 2018

If we're going to do something like this, I don't think it should be an undocumented side-effect of setting the value to 0. I think we need to either document this behavior or provide a separate keyword argument.

@importepeu
Copy link
Contributor Author

importepeu commented Aug 24, 2018

@cachedout Do you think people who need to touch the file with current date has configured their states with atime="0" and mtime="0" and this PR would brings side effects ? Leaving these options unset is a shorter way to get this result.

But if this PR is allowed, indeed a documentation update should be required too, I agree. But I don't think on the other side that a new keyword would be required.

This is my first Salt PR so I don't really have the required experience back regarding side effects ^^ Let's discuss it :)

@cachedout
Copy link
Contributor

@importepeu No, that's not quite what I mean. I just mean that if setting it to 0 is the intended way to set it to the epoch, we should document that behavior as such.

@importepeu
Copy link
Contributor Author

importepeu commented Aug 29, 2018

Ok, fine. Am I able to update the doc ?

I saw that jenkins jobs for centos7 failed for python2 and python3, does this mean that I need to investigate the error on thoose plateforms ? The code was tested on Ubuntu 16 with python2 and as far as I see, thoose errors don't seem to be directly linked to the updated code...

Traceback (most recent call last):
  File "/tmp/kitchen/testing/tests/support/helpers.py", line 210, in wrap
    raise exc
AssertionError

Traceback (most recent call last):
  File "/tmp/kitchen/testing/tests/integration/shell/test_saltcli.py", line 201, in test_missing_minion
    assert retcode == salt.defaults.exitcodes.EX_GENERIC, retcode
AssertionError: None

Regards,
Guillaume.

@cachedout
Copy link
Contributor

It just needs to be documented in the docstring for the function itself. There is no need to worry about those other errors. They are unrelated to your change.

Add some documentation about possible values, particularly what time will be used when setting atime or mtime to 0 or when unset them.
@importepeu
Copy link
Contributor Author

@cachedout Hi, I've updated the file with some documentation as requested. Please let me know if it's ok. Regards, Guillaume.

@cachedout cachedout merged commit fe35a9e into saltstack:develop Sep 17, 2018
@alexey-zhukovin alexey-zhukovin added the has master-port port to master has been created label May 12, 2020
twangboy pushed a commit to alexey-zhukovin/salt that referenced this pull request Nov 14, 2022
Ch3LL pushed a commit that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has master-port port to master has been created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants