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

grains: Use platform.freedesktop_os_release #61589

Merged
merged 6 commits into from
Sep 30, 2022

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented Feb 7, 2022

Python 3.10 introduced platform.freedesktop_os_release which does the same as _parse_os_release("/etc/os-release", "/usr/lib/os-release").

The parsing of the os-release file is very similar and differs only slightly in the used regular expressions. In contrast to
_parse_os_release, platform.freedesktop_os_release caches the result of the parsed os-release file.

Use platform.freedesktop_os_release when available. When salt drops the support for Python < 3.10 (some time in the distant future), _parse_os_release can be removed.

Also update the OS grains test cases to use /etc/os-release data from still supported Debian/Ubuntu releases.

This merge request contains the commits from the merge request #61597. So I recommend to review and merge #61597 first.

@bdrung bdrung requested a review from a team as a code owner February 7, 2022 18:18
@bdrung bdrung requested review from dwoz and removed request for a team February 7, 2022 18:18
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from c6e3bfc to 178b8a8 Compare February 7, 2022 18:39
@bdrung bdrung force-pushed the use_freedesktop_os_release branch 3 times, most recently from b58c854 to 3b2c5e5 Compare February 8, 2022 04:04
@bdrung bdrung changed the title Use platform.freedesktop_os_release grains: Use platform.freedesktop_os_release Feb 8, 2022
@bdrung
Copy link
Contributor Author

bdrung commented Feb 8, 2022

Updated the code so that _parse_os_release throws an OSError if no os-release file can be read.

@bdrung bdrung force-pushed the use_freedesktop_os_release branch 8 times, most recently from 3fe0c4b to a78395c Compare February 10, 2022 13:51
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from a78395c to 76c247e Compare February 10, 2022 23:23
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from 76c247e to fa7775a Compare February 11, 2022 13:49
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from fa7775a to f9932ba Compare September 26, 2022 22:51
@dmurphy18 dmurphy18 self-requested a review September 27, 2022 00:17
@dmurphy18 dmurphy18 added the Sulfur v3006.0 release code name and version label Sep 27, 2022
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from f9932ba to 428aa39 Compare September 27, 2022 08:01
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Looks good, need to get all tests passing, updated against branch and hopefully Windows VM and Apple VM will behave
Ready to approve once all the tests pass

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Need to address Windows issues, no /etc/os-release on Windows
Restarted MacOS test, but nest check not same issue as on Windows

@bdrung bdrung force-pushed the use_freedesktop_os_release branch from c9cca87 to 7ab5d4b Compare September 27, 2022 23:05
@bdrung
Copy link
Contributor Author

bdrung commented Sep 27, 2022

freedesktop_os_release shouldn't be called on Windows/Mac. I changed this function to a private function named _freedesktop_os_release. Let's see if that solves the test failure.

Change `_parse_os_release` to behave nearly identical to
`platform.freedesktop_os_release` from Python >= 3.10, if called with
("/etc/os-release", "/usr/lib/os-release").

There are no os-release files for Pop!_OS and therefore the test cases
would fail with an `OSError` now.

Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Python 3.10 introduced `platform.freedesktop_os_release` which does the
same as `_parse_os_release("/etc/os-release", "/usr/lib/os-release")`.

The parsing of the os-release files is nearly identical and differs only
slightly in the used regular expressions. In contrast to
`_parse_os_release`, `platform.freedesktop_os_release` caches the result
of the parsed os-release file.

Use `platform.freedesktop_os_release` when available. When salt drops
the support for Python < 3.10 (some time in the distant future),
`_parse_os_release` can be removed.

Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Update the OS grains test cases to use `/etc/os-release` data from still
supported Debian/Ubuntu releases.

Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Update the OS grains test cases to use `/etc/os-release` data from the
latest Rocky Linux release. Data is taken from Docker image
`rockylinux:8`.

Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Update the OS grains test cases to use `/etc/os-release` data from the
latest AlmaLinux release. Data is taken from docker image `almalinux:8`.

Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
@bdrung bdrung force-pushed the use_freedesktop_os_release branch from 7ab5d4b to 13a5f11 Compare September 30, 2022 00:10
@bdrung
Copy link
Contributor Author

bdrung commented Sep 30, 2022

Making freedesktop_os_release private solved it. All test runs pass except for pr-windows-2016-x64-py3-pytest that has four failing tests with FactoryNotStarted, but that looks unrelated to my merge request.

@Ch3LL Ch3LL merged commit d93c400 into saltstack:master Sep 30, 2022
@bdrung bdrung deleted the use_freedesktop_os_release branch September 30, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants