salt
salt copied to clipboard
Add exception for Arch in timezone zone_compare
Resubmission of #61390
Like FreeBSD an Archlinux host may not have a /etc/localtime symlink yet which can cause a comparison error during provisioning. It would be nice to not have to work around this problem any more with this small addition.
If tests are required I could use a hand writing a suitable one.
What does this PR do?
Prevents zone_compare
in salt/modules/timezone.py
throwing an error when /etc/localtime is missing on Archlinux hosts.
What issues does this PR fix or reference?
Fixes: #61296
Previous Behavior
[ERROR ] Unable to compare desired timezone 'Australia/Melbourne' to system timezone: Failed to read /etc/localtime to determine current timezone: No such file or directory
local:
----------
ID: Australia/Melbourne
Function: timezone.system
Result: False
Comment: Unable to compare desired timezone 'Australia/Melbourne' to system timezone: Failed to read /etc/localtime to determine current timezone: No such file or directory
Started: 06:37:43.845836
Duration: 0.653 ms
Changes:
Summary for local
------------
Succeeded: 0
Failed: 1
------------
Total states run: 1
Total run time: 0.653 ms
New Behavior
local:
----------
ID: Australia/Melbourne
Function: timezone.system
Result: True
Comment: Set timezone Australia/Melbourne
Started: 06:38:29.172314
Duration: 78.068 ms
Changes:
----------
timezone:
Australia/Melbourne
Summary for local
------------
Succeeded: 1 (changed=1)
Failed: 0
------------
Total states run: 1
Total run time: 78.068 ms
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
- [ ] Docs
- [ ] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [ ] Tests written/updated
Commits signed with GPG?
Yes/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.
Been trying to get this officially fixed for a while, any help or guidance on how to make this happen would be greatly appreciated.
"Arch"
by itself is Truthy, so this is not an exception for Arch but a check for all (supported) OSes. I'm not too familiar with Salt development yet, but that is something you could probably have caught by including (OS-specific) tests in the PR. I don't understand why a missing file should be treated differently on some OSes in the first place and I see this also came up in the referenced issue.
The special cases for AIX/Solaris are documented, but this one (whether for FreeBSD/Arch only or as a general check) is not. The docstrings indicate that the whole module was originally written for /etc/localtime only. For this function, it could be clarified that it "Compares the given timezone name with the system timezone name. [On platforms using /etc/localtime, if this file exists, also] checks the hash sum ..."
You're right that the statement was Truthy. I've fixed this in a new push but I have not been able to write tests.
Is there any 'gotcha' on why this section cannot just be:
if not os.path.isfile(_get_localtime_path()):
return timezone == get_zone()
Without an OS check?
Thank you Ch3LL, I'm in a bit of a pickle at the moment with time management but I'll try and get this done
k, i'm more than willing to help write the tests, so let me know if you can't find the time.
Sorry I won't be able to take care of this one at the current time. Despite how badly I'm itching to get this problem fixed.
@ipaqmaster are you interested in adding the tests and getting this merged in?
He previously stated he doesn't have the time. I'm willing to add some tests coverage here unless you want to tackle this in your test clinic @waynew .
Just let me know
@Ch3LL I probably won't be able to tackle this before mid Nov/Dec, so if you get to it before that, feel free to take it on!
I went ahead and pushed some test coverage, and made the requested changes.
@waynew and @ipaqmaster can you give it a look and see if everything looks good.