SEP: Expose utility functions to states, but not to CLI or template context

[terminalmage]

[OrangeDog]

Can we examine why __utils__ is planned for removal, and whether perhaps

• a) it shouldn't be, and this can then use it
• b) that implies that option 2 is also a bad idea

[Ch3LL]

@s0undt3ch and @dwoz can you take a look at this

[terminalmage]

Here's a patch that implements this SEP, including automated tests to confirm it.

I even changed the implementation from the initial proposal. The utility functions are kept as private funcs, meaning that we don't have to worry about modifying the docs to hide them, nor deal with modifying the pre-commit check which looks for CLI examples in public functions. Even better!

(Note that this wouldn't be the full implementation. Since the functions argument is no longer used in salt.loader.states(), a full implementation would probably involve removing those arguments from invocations of salt.loader.states() and updating related test cases.)

The added functionality is simple, and the test cases are similarly easy-to-understand.

Using my implementation:

• We can continue to rely on loader membership (as Salt has for more than 10 years) to check if there is a platform-specific function implemented for a given platform.
• Utility functions in execution modules can be invoked from state modules without them being unnecessarily exposed to the CLI.
• We don't have to add additional layers of abstraction to prevent code duplication.
• We don't need to need to manually perform opts checks to recreate the providers functionality already provided by the loader.

Without my implementation, the position of the project is that loader membership checks are "techincal debt". To fix that technical debt, each platform-specific function referenced in the pkg/pkgrepo state modules, which is not useful on the CLI, would need to be moved to a utils module (in many cases creating a whole new utils module for this purpose). Since this "fix" would remove that function from the CLI, this removal would require that the function be deprecated prior to removal. Additionally, to prevent code duplication in the __virtual__ function, the platform checks done in the __virtual__ would also need to be moved to a utils module. This method also requires that we pass in __opts__ and __grains__ to all invocations of the utils function that performs the platform check, both in the execution module's __virtual__ and in any place where we want to do a platform check in the state module. (NOTE: This wouldn't technically be necessary unless #66 is implemented. But, if we don't pass in the __opts__ and __grains__, and then #66 is approved and implemented, part of the implementation of #66 would require that we revisit the code in these utils modules. Each function that needs __opts__ or __grains__ would need be updated so that __opts__ and __grains__ are passed in, and all invocations of these functions would also need to be updated to pass them. A number of test cases would also need to be updated.)

Please correct me if any of the details in the above paragraph are inaccurate, I don't want to be seen as misrepresenting things.

It has been argued that my proposal adds to Salt's maintenance burden. I've shown with my patch just how elegant this solution is, and how easy it is to test. It also saves the project a lot of technical debt, as loader membership checks would not need to be removed.

The alternative appears to be far more burdensome. If I am mistaken in my analysis of the alternative, I welcome being proven wrong, because the more-elegant solution will win and Salt will be better for it.

[cmcmarrow]

@terminalmage What do you think of this idea, where we move the check to its own util.

salt.utils.portage_loader.py

import os
import sys

HAS_PORTAGE = False
try:
    import portage

    HAS_PORTAGE = True
except ImportError:
    if os.path.isdir("/usr/lib/portage/pym"):
        try:
            # In a virtualenv, the portage python path needs to be manually added
            sys.path.insert(0, "/usr/lib/portage/pym")
            import portage

            HAS_PORTAGE = True
        except ImportError:
            pass


def portage_status():
    return HAS_PORTAGE and __grains__["os"] == "Gentoo"

salt/modules/ebuildpkg.py

import copy
import datetime
import logging
import os
import re

import salt.utils.args
import salt.utils.compat
import salt.utils.data
import salt.utils.functools
import salt.utils.path
import salt.utils.pkg
import salt.utils.systemd
import salt.utils.versions
from salt.exceptions import CommandExecutionError, MinionError
import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

log = logging.getLogger(__name__)

# Define the module's virtual name
__virtualname__ = "pkg"


def __virtual__():
    """
    Confirm this module is on a Gentoo based system
    """
    if salt.utils.portage_loader.portage_status():
        return __virtualname__
    return (
        False,
        "The ebuild execution module cannot be loaded: either the system is not Gentoo"
        " or the portage python library is not available.",
    )
# ...

salt.utils.other.py

import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

# Define the module's virtual name
__virtualname__ = "other"


def __virtual__():
    """
    Confirm this module is on a Gentoo based system
    """
    if salt.utils.portage_loader.portage_status():
        return __virtualname__
    return (
        False,
        "No Portage"
    )
# ...

[terminalmage]

Given that this function would presumably need to be called from other places that are unrelated a virtual function, perhaps a better name for the function would be has_portage.

[cmcmarrow]

I dont see anything wrong with the name change, plus we can add portage_virtual

salt.utils.has_portage.py

import os
import sys

HAS_PORTAGE = False
try:
    import portage

    HAS_PORTAGE = True
except ImportError:
    if os.path.isdir("/usr/lib/portage/pym"):
        try:
            # In a virtualenv, the portage python path needs to be manually added
            sys.path.insert(0, "/usr/lib/portage/pym")
            import portage

            HAS_PORTAGE = True
        except ImportError:
            pass


def portage_status():
    return HAS_PORTAGE and __grains__["os"] == "Gentoo"


def portage_virtual(virtual_name, failed_message=None):
    if portage_status():
        return virtual_name
    if failed_message is None:
        failed_message = f"{virtual_name} cannot be loaded: either the system is not Gentoo\n or the portage python library is not available."
    return False, failed_message

import copy
import datetime
import logging
import os
import re

import salt.utils.args
import salt.utils.compat
import salt.utils.data
import salt.utils.functools
import salt.utils.path
import salt.utils.pkg
import salt.utils.systemd
import salt.utils.versions
from salt.exceptions import CommandExecutionError, MinionError
import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

log = logging.getLogger(__name__)

# Define the module's virtual name
__virtualname__ = "pkg"


def __virtual__():
    return salt.utils.portage_virtual(__virtualname__)
# ...