salt icon indicating copy to clipboard operation
salt copied to clipboard

Add exception for Arch in timezone zone_compare

Open ipaqmaster opened this issue 2 years ago • 5 comments

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.

ipaqmaster avatar Jun 06 '22 06:06 ipaqmaster

Been trying to get this officially fixed for a while, any help or guidance on how to make this happen would be greatly appreciated.

ipaqmaster avatar Jun 07 '22 23:06 ipaqmaster

"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 ..."

raymonad avatar Jun 09 '22 08:06 raymonad

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?

ipaqmaster avatar Jun 13 '22 08:06 ipaqmaster

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

ipaqmaster avatar Sep 21 '22 00:09 ipaqmaster

k, i'm more than willing to help write the tests, so let me know if you can't find the time.

Ch3LL avatar Sep 21 '22 18:09 Ch3LL

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 avatar Oct 10 '22 04:10 ipaqmaster

@ipaqmaster are you interested in adding the tests and getting this merged in?

waynew avatar Oct 17 '22 18:10 waynew

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 avatar Oct 17 '22 19:10 Ch3LL

@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!

waynew avatar Oct 20 '22 23:10 waynew

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.

Ch3LL avatar Nov 01 '22 19:11 Ch3LL