Skip to content

Use os-release ID_LIKE for the os-family grain#60171

Closed
evelikov wants to merge 2 commits intosaltstack:masterfrom
evelikov:id_like-based-os_family
Closed

Use os-release ID_LIKE for the os-family grain#60171
evelikov wants to merge 2 commits intosaltstack:masterfrom
evelikov:id_like-based-os_family

Conversation

@evelikov
Copy link
Copy Markdown

What does this PR do?

This MR parses the ID_LIKE token in the os-release file and uses it for the os-family grain, before falling back to the old heuristics.

As result this paves the way to more scale-able Linux distribution handling. In particular, today each new distro needs an entry in _OS_FAMILY_MAP (and potentially _OS_NAME), while the information is readily available within os-release. It also opens the path to cleanup the parsing of non os-release release files and remove the vast part of _OS_NAME_MAP and _OS_FAMILY_MAP.

But all those can follow in due course.

What issues does this PR fix or reference?

Fixes:

Previous Behavior

os-family grain is used based on the os grain and _OS_FAMILY_MAP

New Behavior

os-family grain is set to a valid distribution entry, as based on ID_LIKE and _OS_NAME_MAP. If missing or invalid, we fallback to the old behaviour.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

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

@evelikov evelikov requested a review from a team as a code owner May 11, 2021 16:15
@evelikov evelikov requested review from twangboy and removed request for a team May 11, 2021 16:15
@welcome
Copy link
Copy Markdown

welcome bot commented May 11, 2021

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@evelikov
Copy link
Copy Markdown
Author

Please note, that the change itself is an internal detail and should not produce any visible changes. Thus I did not change the tests nor did I add any docs/changelog. More than happy to do as if needed.

@evelikov
Copy link
Copy Markdown
Author

Can anyone share why the CI is failing - the output is truncated :-\

@xmsk
Copy link
Copy Markdown

xmsk commented May 15, 2021

Can anyone share why the CI is failing - the output is truncated :-\

If you click on 'Details' on the failing job all the way at the bottom there is a link to the Jenkins build where you can check the console output, e.g. https://jenkins.saltproject.io/job/pr-ubuntu-2004-amd64-py3-pytest/job/PR-60171/1/console

evelikov added 2 commits May 20, 2021 11:54
The os-release ID_LIKE property, defines what we essentially call
"os_family". It can be a space separated list of IDs, starting with the
most relevant one.

Use it when possible and fall-back to the _OS_FAMILY_MAP otherwise.

Looking into the future...
Since os-release is a hard-requirement for nearly all modern Linux
distributions, we could we could make it the canonical source at some
point in the future.

By doing so, we can remove all the distro specific *release files as
well as lsb-release parsing. This will allow us to further simplify
things by removing the majority of _OS_FAMILY_MAP. And if we start
parsing the os-release ID, most of _OS_NAME_MAP can also be removed ;-)

But all that for another day.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
These two platforms consistently set the os-release ID_LIKE properly
which is handled with last commit. So drop the explicit entries from the
table.

In theory, nearly all Linux entries could be dropped although left for a
later day. Enterprise distros et al, are less flexible to provide their
os-release package making the audit process tad hard.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
@evelikov evelikov force-pushed the id_like-based-os_family branch from 3fffb95 to 6752be9 Compare May 20, 2021 10:56
@evelikov evelikov mentioned this pull request May 23, 2021
3 tasks
@evelikov
Copy link
Copy Markdown
Author

The logs are not so helpful why things are failing. Especially in the macosx case - the series does not touch any non-linux code.
Can anyone guide me in the right direction? Thanks

Copy link
Copy Markdown
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

This needs a changelog and a test case

@twangboy twangboy added needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases needs-changelog labels Oct 21, 2022
@evelikov
Copy link
Copy Markdown
Author

@twangboy do you have any PR or anything else I can use as a reference? Also happy 1 year anniversary :-P

@dwoz
Copy link
Copy Markdown
Contributor

dwoz commented Dec 10, 2023

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

@dwoz dwoz closed this Dec 10, 2023
@dwoz dwoz added help-wanted Community help is needed to resolve this Abandoned labels Dec 10, 2023
@evelikov
Copy link
Copy Markdown
Author

Fwiw my earlier question still stands:

What tests are expected, is there a PR or documentation that can be used as a reference?

@dmurphy18
Copy link
Copy Markdown
Contributor

@evelikov The following command gives lots of examples of testing grains, cut & paste & manipulate for the test to exercise your code changes. Testing a grain is pretty simple.

david@david-XPS-15-9570:~/devcode/dgm_salt_3006/salt$ find tests/pytests/ -name "test*grains*"
tests/pytests/integration/ssh/test_grains.py
tests/pytests/integration/modules/saltutil/test_grains.py
tests/pytests/pkg/integration/test_salt_grains.py
tests/pytests/functional/grains/test_grains.py
tests/pytests/unit/states/test_grains.py
tests/pytests/unit/modules/test_grains.py
david@david-XPS-15-9570:~/devcode/dgm_salt_3006/salt$ 

For your changes, recommend doing a unit test since very simple change.
Also the changelog can be a one-liner, similar to a git commit string, look at other for examples:

@evelikov
Copy link
Copy Markdown
Author

@dmurphy18 thanks for the pointers. Looking through all those files - neither seem relevant to what's being achieved here.

At a glance, the code should already be tested via the existing grains in ./tests/pytests/unit/grains/test_core.py. So in practise the code is already covered by the existing tests. Or am I missing something?

In theory one can continue chipping/removing the hard-coded mappings in salt/grains/core.py, while ensuring the existing tests pass. And potentially even remove the non os-release paths, at the very end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Abandoned help-wanted Community help is needed to resolve this needs-changelog needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants