[FEATURE REQUEST] Incorporate lint for log messages in __virtual__

[oeuftete]

Is your feature request related to a problem? Please describe.

Community contributors (and maybe Salt devs, too) add well-meaning logs to __virtual__ methods, typically to highlight that a module could not be loaded. However, these messages will show up very frequently in the logs, which is especially problematic if the logs are errors (see, e.g. #57723). The supported solution is to return a tuple (False, 'the log message') from __virtual__.

Describe the solution you'd like

Use pylint to detect use of logging in __virtual__ methods. Enforce this via CI and in pre-commit.

Describe alternatives you've considered

Status quo.

[oeuftete]

Support was added for this in saltstack/salt-pylint#37. However, I ran into a couple of obstacles trying to add it.

Salt's dunders blow up the check

E.g.

salt/salt/modules/linux_service.py

Lines 18 to 56 in b95213e

	def __virtual__():
	"""
	Only work on systems which exclusively use sysvinit
	"""
	# Disable on these platforms, specific service modules exist:
	disable = set(
	(
	"RedHat",
	"CentOS",
	"Amazon",
	"ScientificLinux",
	"CloudLinux",
	"Fedora",
	"Gentoo",
	"Ubuntu",
	"Debian",
	"Devuan",
	"ALT",
	"OEL",
	"Linaro",
	"elementary OS",
	"McAfee OS Server",
	"Raspbian",
	"SUSE",
	)
	)
	if __grains__.get("os") in disable:
	return (False, "Your OS is on the disabled list")
	# Disable on all non-Linux OSes as well
	if __grains__["kernel"] != "Linux":
	return (False, "Non Linux OSes are not supported")
	init_grain = __grains__.get("init")
	if init_grain not in (None, "sysvinit", "unknown"):
	return (False, "Minion is running {0}".format(init_grain))
	elif __utils__["systemd.booted"](__context__):
	# Should have been caught by init grain check, but check just in case
	return (False, "Minion is running systemd")
	return "service"

$ pylint --rcfile=.pylintrc --disable=I --rcfile=.pylintrc --disable=I salt/modules/linux_service.py
Traceback (most recent call last):
  File "/home/ken/.virtualenvs/salt/bin/pylint", line 8, in <module>
    sys.exit(run_pylint())
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/__init__.py", line 23, in run_pylint
    PylintRun(sys.argv[1:])
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/lint.py", line 1731, in __init__
    linter.check(args)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/lint.py", line 1004, in check
    self._do_check(files_or_modules)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/lint.py", line 1165, in _do_check
    self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/lint.py", line 1252, in check_astroid_module
    walker.walk(ast_node)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 74, in walk
    callback(astroid)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/saltpylint/virt.py", line 52, in visit_functiondef
    for inferred in functions.func.expr.infer():
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/astroid/decorators.py", line 131, in raise_if_nothing_inferred
    yield next(generator)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/astroid/decorators.py", line 92, in wrapped
    generator = _func(node, context, **kwargs)
  File "/home/ken/.virtualenvs/salt/lib/python3.6/site-packages/astroid/inference.py", line 195, in infer_name
    name=self.name, scope=self.scope(), context=context
astroid.exceptions.NameInferenceError: '__grains__' not found in <FunctionDef.__virtual__ l.18 at 0x7f37b4840eb8>.

Log message in missing import blocks aren't found

salt/salt/returners/appoptics_return.py

Lines 83 to 108 in b95213e

	# Import third party libs
	try:
	import appoptics_metrics
	HAS_APPOPTICS = True
	except ImportError:
	HAS_APPOPTICS = False
	# Define the module's Virtual Name
	__virtualname__ = "appoptics"
	log = logging.getLogger(__name__)
	def __virtual__():
	if not HAS_APPOPTICS:
	log.error(
	"The appoptics_return module couldn't load the appoptics_metrics module."
	)
	log.error("please make sure it is installed and is in the PYTHON_PATH.")
	return (
	False,
	"Could not import appoptics_metrics module; "
	"appoptics-metrics python client is not installed.",
	)
	return __virtualname__

$ pylint --rcfile=.pylintrc --disable=I --rcfile=.pylintrc --disable=I salt/returners/appoptics_return.py 

------------------------------------
Your code has been rated at 10.00/10

If I add:

    log.info('foo')

at the top of __virtual__, though:

$ pylint --rcfile=.pylintrc --disable=I --rcfile=.pylintrc --disable=I salt/returners/appoptics_return.py 
************* Module salt.returners.appoptics_return
salt/returners/appoptics_return.py:98: [E1401(log-in-virtual), __virtual__] Log statement detected inside __virtual__ function. Remove it.

-----------------------------------
Your code has been rated at 9.29/10

[oeuftete]

Opened saltstack/salt-pylint#42, which hopefully will help move this forward. This is blocked until that's fixed.