Skip to content

Fix Pop!_OS 20.04 not using aptpkg - #58443

Merged
Ch3LL merged 1 commit into
saltstack:masterfrom
thehunmonkgroup:fix-popos-20.04
Feb 2, 2021
Merged

Fix Pop!_OS 20.04 not using aptpkg#58443
Ch3LL merged 1 commit into
saltstack:masterfrom
thehunmonkgroup:fix-popos-20.04

Conversation

@thehunmonkgroup

@thehunmonkgroup thehunmonkgroup commented Sep 13, 2020

Copy link
Copy Markdown
Contributor

What does this PR do?

Pop!_OS 20.04 not recognized as aptpkg for pkg states

What issues does this PR fix or reference?

Fixes: #58395

Previous Behavior

aptpkg is not selected for pkg states

New Behavior

aptpkg is selected for pkg states

Merge requirements satisfied?

Commits signed with GPG?

No

NOTE: I'm far from a Salt developer, and just trying to get the ball rolling here.

It's unclear if a test is necessary, it didn't look like most other stuff in the _OS_FAMILY_MAP had one.

By a quick test, this patch now reports the correct os and os_family:

[:~] $ salt-call grains.get os_family
local:
    Debian
[:~] $ salt-call grains.get os
local:
    Pop

And pkg.installed also works.

@thehunmonkgroup
thehunmonkgroup requested a review from a team as a code owner September 13, 2020 17:14
@ghost
ghost requested review from garethgreenaway and removed request for a team September 13, 2020 17:14
@ids1024

ids1024 commented Sep 16, 2020

Copy link
Copy Markdown

Perhaps salt should check for ID_LIKE in /etc/os-release: https://www.freedesktop.org/software/systemd/man/os-release.html

That could make it more robust for running on various distros, without necessarily hard-coding each distro's ID.

@dwoz dwoz added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 22, 2020
@sagetherage sagetherage added bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around labels Oct 1, 2020
@ScriptAutomate

Copy link
Copy Markdown
Contributor

This has fixed my inability to manage Pop!_OS systems, and I find myself applying this change manually to each update of Salt. I haven't tested on Pop!_OS 20.10, but have on 20.04.

I think this would be a good fix/update to apply to Salt, and then have a separate issue regarding the suggestion by @ids1024 in a future update to Salt for better general reach across different OS spinoffs.

See new issue: #59061

To fulfill the testcase requirement label, what extra information needs to be provided/included in this PR that would help achieve the coverage?

@s0undt3ch

Copy link
Copy Markdown
Contributor

We need at least a Unit test case which ensures PopOS is properly identified

Comment thread changelog/58395.fixed Outdated

@ScriptAutomate ScriptAutomate left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we could introduce valuable unit testing for this, since we are only extending two dictionaries to provide an Ubuntu-derivative the ability to call aptpkg.* and pkg.* execution and state modules.

This would allow for Salt to execute package management on Pop!_OS, and #59061 being addressed will assist in the default reach of Salt on wider derivatives without hardcoding into Salt.

UPDATE: Based on @whytewolf's response, @thehunmonkgroup see below comments for guidance on unit test guidance for this PR

@whytewolf

Copy link
Copy Markdown
Collaborator

actually writing a test for this wouldn't be that difficult. most of the work is already done,

Just add a new stanza pretty much like this one in the test_core.py

https://github.com/saltstack/salt/blob/master/tests/unit/grains/test_core.py#L690-L707

@ScriptAutomate

ScriptAutomate commented Dec 16, 2020

Copy link
Copy Markdown
Contributor

actually writing a test for this wouldn't be that difficult. most of the work is already done,

Just add a new stanza pretty much like this one in the test_core.py

https://github.com/saltstack/salt/blob/master/tests/unit/grains/test_core.py#L690-L707

Ah! Got it, so based on that, we'd want something like the following added to test_core.py (values are based on grains output from Pop!_OS systems I tried against):

    @skipIf(not salt.utils.platform.is_linux(), "System is not Linux")
    def test_pop_focal_os_grains(self):
        """
        Test if OS grains are parsed correctly in Pop!_OS 20.04 "Focal Fossa"
        """
        _os_release_map = {
            "_linux_distribution": ("Pop", "20.04", "focal"),
        }
        expectation = {
            "os": "Pop",
            "os_family": "Debian",
            "oscodename": "focal",
            "osfullname": "Pop",
            "osrelease": "20.04",
            "osrelease_info": (20, 4),
            "osmajorrelease": 20,
            "osfinger": "Pop-20",
        }
        self._run_os_grains_tests("pop-20.04", _os_release_map, expectation)

    @skipIf(not salt.utils.platform.is_linux(), "System is not Linux")
    def test_pop_groovy_os_grains(self):
        """
        Test if OS grains are parsed correctly in Pop!_OS 20.10 "Groovy Gorilla"
        """
        _os_release_map = {
            "_linux_distribution": ("Pop", "20.10", "groovy"),
        }
        expectation = {
            "os": "Pop",
            "os_family": "Debian",
            "oscodename": "groovy",
            "osfullname": "Pop",
            "osrelease": "20.10",
            "osrelease_info": (20, 10),
            "osmajorrelease": 20,
            "osfinger": "Pop-20",
        }
        self._run_os_grains_tests("pop-20.10", _os_release_map, expectation)

@whytewolf

Copy link
Copy Markdown
Collaborator

actually writing a test for this wouldn't be that difficult. most of the work is already done,
Just add a new stanza pretty much like this one in the test_core.py
https://github.com/saltstack/salt/blob/master/tests/unit/grains/test_core.py#L690-L707

Ah! Got it, so based on that, we'd want something like the following added to test_core.py (values are based on grains output from Pop!_OS systems I tried against):

    @skipIf(not salt.utils.platform.is_linux(), "System is not Linux")
    def test_pop_focal_os_grains(self):
        """
        Test if OS grains are parsed correctly in Pop!_OS 20.04 "Focal Fossa"
        """
        _os_release_map = {
            "_linux_distribution": ("Pop", "20.04", "focal"),
        }
        expectation = {
            "os": "Pop",
            "os_family": "Debian",
            "oscodename": "focal",
            "osfullname": "Pop",
            "osrelease": "20.04",
            "osrelease_info": (20, 4),
            "osmajorrelease": 20,
            "osfinger": "Pop-20",
        }
        self._run_os_grains_tests("pop-20.04", _os_release_map, expectation)

    @skipIf(not salt.utils.platform.is_linux(), "System is not Linux")
    def test_pop_groovy_os_grains(self):
        """
        Test if OS grains are parsed correctly in Pop!_OS 20.10 "Groovy Gorilla"
        """
        _os_release_map = {
            "_linux_distribution": ("Pop", "20.10", "groovy"),
        }
        expectation = {
            "os": "Pop",
            "os_family": "Debian",
            "oscodename": "groovy",
            "osfullname": "Pop",
            "osrelease": "20.10",
            "osrelease_info": (20, 10),
            "osmajorrelease": 20,
            "osfinger": "Pop-20",
        }
        self._run_os_grains_tests("pop-20.10", _os_release_map, expectation)

yeap, in theory. @waynew would know if I am leading you down the wrong path.

@thehunmonkgroup

Copy link
Copy Markdown
Contributor Author

I tossed in the tests suggested by @whytewolf , hopefully that will be enough?

@s0undt3ch s0undt3ch added Aluminium Release Post Mg and Pre Si and removed needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels Jan 4, 2021
@Ch3LL
Ch3LL merged commit 2a373c5 into saltstack:master Feb 2, 2021
@welcome

welcome Bot commented Feb 2, 2021

Copy link
Copy Markdown

Congratulations on your first PR being merged! 🎉

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

Labels

Aluminium Release Post Mg and Pre Si bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Pop!_OS 20.04 not recognized as aptpkg for pkg states

8 participants