ci: run pylint on ci build

.github/workflows/build.yml

@@ -30,7 +30,10 @@ jobs:
 
     steps:
     - name: Install required PyPI packages
-      run: ${{ matrix.pip-command }} install "ansible-lint>=6.0.0,<7.0.0"
+      run: ${{ matrix.pip-command }} install ansible-lint pylint mock paramiko psycopg2-binary websockify websocket-client looseversion isort pycodestyle pyflakes
+
+    - name: Ensure required packages are available for linting
+      run: dnf -y install otopi python3-ovirt-setup-lib python3-daemon
 
     - name: Checkout sources
       uses: ovirt/checkout-action@main
@@ -39,6 +42,9 @@ jobs:
       run: |
         .automation/build-rpm.sh $ARTIFACTS_DIR
 
+    - name: Run python check
+      run: build/python-check.sh
+
     - name: Create DNF repository
       run: |
         createrepo_c $ARTIFACTS_DIR

.gitignore

@@ -80,12 +80,12 @@ packaging/services/ovirt-engine/ovirt-engine.conf
 packaging/services/ovirt-engine/ovirt-engine.py
 packaging/services/ovirt-engine/ovirt-engine.systemd
 packaging/services/ovirt-engine/ovirt-engine.sysv
-packaging/services/ovirt-fence-kdump-listener/config.py
+packaging/services/ovirt-fence-kdump-listener/fence_kdump_config.py
 packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.conf
 packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py
 packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.systemd
 packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.sysv
-packaging/services/ovirt-websocket-proxy/config.py
+packaging/services/ovirt-websocket-proxy/websocket_config.py
 packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.conf
 packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.py
 packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.systemd

.pylintrc

@@ -0,0 +1,13 @@
+[MASTER]
+init-hook=
+  import sys, pathlib;
+  root = pathlib.Path().resolve();
+  paths = [
+    root / "packaging" / "pythonlib",
+    root / "packaging" / "services" / "ovirt-engine",
+    root / "packaging" / "services" / "ovirt-fence-kdump-listener",
+    root / "packaging" / "services" / "ovirt-websocket-proxy",
+    root / "packaging" / "services" / "ovirt-engine-notifier",
+    root / "packaging" / "libexec" / "ovirt-vmconsole-proxy-helper"
+  ];
+  [sys.path.insert(0, str(p)) for p in paths if str(p) not in sys.path]

Dockerfile

@@ -23,7 +23,10 @@ RUN make ovirt-engine.spec
 RUN dnf -y builddep ovirt-engine.spec
 
 # Install run deps
-RUN dnf -y install python3-daemon python3-otopi python3-psycopg2 python3-ovirt-setup-lib otopi-common initscripts-service bind-utils postgresql ovirt-engine-wildfly-overlay mailcap ansible-runner ansible-collection-ansible-posix ovirt-imageio-daemon novnc ovirt-engine-websocket-proxy
+RUN dnf -y install python3-daemon python3-otopi python3-psycopg2 python3-ovirt-setup-lib otopi-common initscripts-service bind-utils postgresql ovirt-engine-wildfly-overlay mailcap ansible-runner ansible-collection-ansible-posix ovirt-imageio-daemon novnc ovirt-engine-websocket-proxy python3-pip
+
+# Install linting tools
+RUN pip install pylint pyflakes isort pycodestyle mock
 
 # engine-setup needs the link to initctl
 RUN ln -s /usr/sbin/service /usr/bin/initctl

Makefile

@@ -30,10 +30,11 @@ PACKAGE_NAME=ovirt-engine
 ENGINE_NAME=$(PACKAGE_NAME)
 MVN=mvn
 PYTHON=$(shell which python3 2> /dev/null)
-PYFLAKES=$(shell which pyflakes 2> /dev/null)
+PYFLAKES=pyflakes
 PY_VERSION=3
-PEP8=$(shell which pycodestyle-3 2> /dev/null)
+PEP8=pycodestyle
 ISORT=isort
+PYLINT=pylint
 PREFIX?=/usr/local
 LOCALSTATE_DIR=$(PREFIX)/var
 BIN_DIR=$(PREFIX)/bin
@@ -184,6 +185,7 @@ BUILD_TARGET=install
 	-e "s|@PEP8@|$(PEP8)|g" \
 	-e "s|@PYFLAKES@|$(PYFLAKES)|g" \
 	-e "s|@ISORT@|$(ISORT)|g" \
+	-e "s|@PYLINT@|$(PYLINT)|g" \
 	-e "s|@DEVMODE@|$(BUILD_DEV)|g" \
 	-e "s|@VMCONSOLE_SYSCONF_DIR@|$(VMCONSOLE_SYSCONF_DIR)|g" \
 	-e "s|@VMCONSOLE_PKI_DIR@|$(VMCONSOLE_PKI_DIR)|g" \
@@ -235,12 +237,12 @@ GENERATED = \
 	packaging/services/ovirt-engine/ovirt-engine.py \
 	packaging/services/ovirt-engine/ovirt-engine.systemd \
 	packaging/services/ovirt-engine/ovirt-engine.sysv \
-	packaging/services/ovirt-fence-kdump-listener/config.py \
+	packaging/services/ovirt-fence-kdump-listener/fence_kdump_config.py \
 	packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.conf \
 	packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py \
 	packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.systemd \
 	packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.sysv \
-	packaging/services/ovirt-websocket-proxy/config.py \
+	packaging/services/ovirt-websocket-proxy/websocket_config.py \
 	packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.conf \
 	packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.py \
 	packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.systemd \

build/helptag-oneline-check.py.in

@@ -48,7 +48,7 @@ def findTags(filename):
     1 per line.
     """
     if filename.endswith('.java') and os.path.isfile(filename):
-        with open(filename, 'r') as f:
+        with open(filename, 'r', encoding='utf-8') as f:
             for line in f:
                 m = __RE_HELPTAG.match(line)
                 if "HelpTagType." in line and not m:

build/helptag.py.in

@@ -90,7 +90,7 @@ def loadTagsFromMappingFile(file):
     state = "HEADER"
     comment = ''
 
-    with open(file, 'r') as f:
+    with open(file, 'r', encoding='utf-8') as f:
         for line in f.readlines():
 
             if state == "HEADER":
@@ -128,7 +128,7 @@ def loadTagsFromCodebase(filename):
     tags = {}
 
     if filename.endswith('.java') and os.path.isfile(filename):
-        with open(filename, 'r') as f:
+        with open(filename, 'r', encoding='utf-8') as f:
             for line in f:
                 comment = ""
                 m = __RE_HELPTAG.match(line)

build/helptag_checker.py.in

@@ -59,7 +59,7 @@ def walkSource(sourcedir):
             if fname not in SKIP_FILES:
                 filename = os.path.join(parent, fname)
                 if filename.endswith('.java') and os.path.isfile(filename):
-                    with open(filename, 'r') as f:
+                    with open(filename, 'r', encoding='utf-8') as f:
                         lines = f.readlines()
                     yield (lines, fname)
 

build/python-check.sh.in

@@ -1,8 +1,9 @@
-#!/bin/sh -x
+#!/bin/sh
 
 PEP8="@PEP8@"
 PYFLAKES="@PYFLAKES@"
 ISORT="@ISORT@"
+PYLINT="@PYLINT@"
 SRCDIR="$(dirname "$0")/.."
 
 cd "${SRCDIR}"
@@ -19,7 +20,7 @@ for exe in "${PYFLAKES}" "${PEP8}"; do
 		echo "WARNING: tool '${exe}' is missing" >&2
 	else
 		if "${exe}" ${FILES}; then
-			echo Passed check: "${exe}" ${FILES}
+			echo Passed check: "${exe}"
 		else
 			echo ERROR: The following check failed:
 			echo "${exe}" ${FILES}
@@ -28,11 +29,23 @@ for exe in "${PYFLAKES}" "${PEP8}"; do
 	fi
 done
 
+if ! which "${PYLINT}" > /dev/null 2>&1; then
+	echo "WARNING: Pylint is missing" >&2
+else
+	if "${PYLINT}" --errors-only ${FILES}; then
+		echo Passed check: "${PYLINT}" --errors-only
+	else
+		echo ERROR: The following check failed:
+		echo python3 -m "${PYLINT}" --errors-only ${FILES}
+		ret=1
+	fi
+fi
+
 if ! which "${ISORT}" > /dev/null 2>&1; then
 	echo "WARNING: tool '${ISORT}' is missing" >&2
 else
 	if "${ISORT}" --check ${FILES}; then
-		echo Passed check: "${ISORT}" --check ${FILES}
+		echo Passed check: "${ISORT}" --check
 	else
 		echo ERROR: The following check failed:
 		echo "${ISORT}" --check ${FILES}

packaging/bin/engine-host-update.py.in

@@ -386,7 +386,7 @@ def upgradeoVirtNodeLegacy(
                 )
 
         secs = 0
-        nonResponsiveCounter = 0
+        non_responsive_counter = 0
         while True:
             print('.', end='')
             state = getHostState(api, name)
@@ -395,8 +395,8 @@ def upgradeoVirtNodeLegacy(
                 break
             elif state in HOST_INSTALL_FAILED_STATES:
                 print('*', end='')
-                nonResponsiveCounter += 1
-                if nonResponsiveCounter >= MAX_NON_RESPONSIVE_COUNT:
+                non_responsive_counter += 1
+                if non_responsive_counter >= MAX_NON_RESPONSIVE_COUNT:
                     raise RuntimeError(
                         'Unable to complete the reinstall operational, '
                         'host is in mode: {0}'.format(state)
@@ -489,7 +489,7 @@ def upgradeoVirtNode(
                 )
 
         secs = 0
-        nonResponsiveCounter = 0
+        non_responsive_counter = 0
         while True:
             print('.', end='')
             state = getHostState(api, name)
@@ -498,8 +498,8 @@ def upgradeoVirtNode(
                 break
             elif state in HOST_INSTALL_FAILED_STATES:
                 print('*', end='')
-                nonResponsiveCounter += 1
-                if nonResponsiveCounter >= MAX_NON_RESPONSIVE_COUNT:
+                non_responsive_counter += 1
+                if non_responsive_counter >= MAX_NON_RESPONSIVE_COUNT:
                     raise RuntimeError(
                         'Unable to complete the reinstall operational, '
                         'host is in mode: {0}'.format(state)
@@ -602,15 +602,15 @@ def processHost(api, name, skipInvalidHostNames):
                 'Invalid host name %s.' % name
             )
 
-    vdsType = host.get_type()
-    print('Type: %s' % vdsType)
+    vds_type = host.get_type()
+    print('Type: %s' % vds_type)
     try:
         state = getHostState(api, name)
         if state == HOST_STATE_UP:
-            if vdsType in OVIRT_NODE_LEGACY_HOST_TYPES:
+            if vds_type in OVIRT_NODE_LEGACY_HOST_TYPES:
                 print('\tPerforming oVirt Node/RHEVH (Legacy) upgrade...')
                 upgradeoVirtNodeLegacy(api, name)
-            elif vdsType in OVIRT_NODE_HOST_TYPES:
+            elif vds_type in OVIRT_NODE_HOST_TYPES:
                 print('\tPerforming oVirt Node NGN upgrade...')
                 upgradeoVirtNode(api, name)
             else:
@@ -639,9 +639,9 @@ def hostsByClusterName(api, name):
     """
     hosts = set()
     query = 'cluster = %s' % name
-    hostObjs = api.hosts.list(query=query)
+    host_objs = api.hosts.list(query=query)
 
-    for host in hostObjs:
+    for host in host_objs:
         hosts.add(host.get_name())
 
     logging.debug(
@@ -852,7 +852,7 @@ if __name__ == '__main__':
                         os.path.expanduser(PASSWORD_FILE)
                     )
                 )
-                with open(os.path.expanduser(PASSWORD_FILE)) as f:
+                with open(os.path.expanduser(PASSWORD_FILE), encoding='utf-8') as f:
                     user, passw = f.readlines()[0].strip().split(':')
                     if user and not username:
                         username = user

packaging/libexec/ovirt-vmconsole-proxy-helper/ovirt-vmconsole-list.py.in

@@ -41,11 +41,7 @@ def urlopen(url, ca_certs=None, verify_host=True):
 
     if getattr(ssl, 'create_default_context', None):
         context = ssl.create_default_context()
-
-        if verify_host:
-            context.check_hostname = ssl.match_hostname
-        else:
-            context.check_hostname = None
+        context.check_hostname = verify_host
 
         if ca_certs:
             context.load_verify_locations(cafile=ca_certs)

packaging/pythonlib/ovirt_engine/configfile.py

@@ -32,13 +32,13 @@ def values(self):
         return self._values
 
     def _loadLine(self, line):
-        emptyMatch = self._EMPTY_LINE.search(line)
-        if emptyMatch is None:
-            keyValueMatch = self._KEY_VALUE_EXPRESSION.search(line)
-            if keyValueMatch is None:
+        empty_match = self._EMPTY_LINE.search(line)
+        if empty_match is None:
+            key_value_match = self._KEY_VALUE_EXPRESSION.search(line)
+            if key_value_match is None:
                 raise RuntimeError(_('Invalid sytax'))
-            self._values[keyValueMatch.group('key')] = self.expandString(
-                keyValueMatch.group('value')
+            self._values[key_value_match.group('key')] = self.expandString(
+                key_value_match.group('value')
             )
 
     def __init__(self, files=[]):
@@ -63,7 +63,7 @@ def loadFile(self, file):
             self.logger.debug("loading config '%s'", file)
             index = 0
             try:
-                with open(file, 'r') as f:
+                with open(file, 'r', encoding='utf-8') as f:
                     for line in f:
                         index += 1
                         self._loadLine(line)
@@ -81,13 +81,13 @@ def loadFile(self, file):
                         line=index,
                         error=e
                     )
-                )
+                ) from e
 
     def expandString(self, value):
         ret = ""
 
         escape = False
-        inQuotes = False
+        in_quotes = False
         index = 0
         while (index < len(value)):
             c = value[index]
@@ -109,9 +109,9 @@ def expandString(self, value):
                     index = i + 1
                     ret += self._values.get(name, "")
                 elif c == '"':
-                    inQuotes = not inQuotes
+                    in_quotes = not in_quotes
                 elif c in (' ', '#'):
-                    if inQuotes:
+                    if in_quotes:
                         ret += c
                     else:
                         index = len(value)

packaging/pythonlib/ovirt_engine/java.py

@@ -49,9 +49,9 @@ def getJavaHome(self):
                 )
             )
 
-        javaHome = stdout[0]
-        self.logger.debug('JAVA_HOME: %s', javaHome)
-        return javaHome
+        java_home = stdout[0]
+        self.logger.debug('JAVA_HOME: %s', java_home)
+        return java_home
 
 
 # vim: expandtab tabstop=4 shiftwidth=4

packaging/pythonlib/ovirt_engine/mem.py

@@ -30,7 +30,7 @@ def get_total_mb():
             (?P<unit>\w+)
         """
     )
-    with open('/proc/meminfo', 'r') as f:
+    with open('/proc/meminfo', 'r', encoding='utf-8') as f:
         content = f.read()
 
     match = _RE_MEMINFO_MEMTOTAL.match(content)

packaging/pythonlib/ovirt_engine/service.py

@@ -168,7 +168,7 @@ def __enter__(self):
                 self._file,
                 os.getpid()
             )
-            with open(self._file, 'w') as f:
+            with open(self._file, 'w', encoding='utf-8') as f:
                 f.write('%s\n' % os.getpid())
 
     def __exit__(self, exc_type, exc_value, traceback):
@@ -180,7 +180,7 @@ def __exit__(self, exc_type, exc_value, traceback):
                 # we may not have permissions to delete pid
                 # so just try to empty it
                 try:
-                    with open(self._file, 'w'):
+                    with open(self._file, 'w', encoding='utf-8'):
                         pass
                 except IOError as e:
                     self.logger.error(
@@ -374,7 +374,7 @@ def daemonAsExternalProcess(
                         _('Had to kill process {pid}').format(
                             pid=p.pid
                         )
-                    )
+                    ) from self.TerminateException
             except OSError as e:
                 self.logger.warning(
                     _('Cannot kill pid {pid}: {error}').format(
@@ -556,7 +556,7 @@ def daemonSetup(self):
 
     def daemonStdHandles(self):
         """Return handles for daemon context"""
-        null = open(os.devnull, 'w')
+        null = open(os.devnull, 'w', encoding='utf-8')
         return (null, null)
 
     def daemonContext(self):

packaging/pythonlib/ovirt_engine/ticket.py

@@ -152,6 +152,7 @@ def decode(self, ticket):
             hasher.update(decoded[field].encode('utf8'))
         digest = hasher.finalize()
         try:
+            res = None
             if 'v2_signature' in decoded:
                 res = pkey.verify(
                     base64.b64decode(decoded['v2_signature']),