CFE-4632: Added python linting in github actions

.github/workflows/ci.yml

@@ -1,19 +1,26 @@
 name: Continuous Integration
 
 on:
+  push:
+    branches: [master]
   pull_request:
-    types: [opened, reopened]
+    branches: [master]
 
 jobs:
   unit_tests:
     name: Run Unit Tests
     runs-on: ubuntu-24.04
+    permissions:
+      contents: read
     steps:
     - uses: actions/checkout@v4
       with:
         submodules: recursive
     - name: Install dependencies
-      run: sudo apt-get update -y && sudo apt-get install -y python3
+      run: |
+        sudo apt-get update -y && sudo apt-get install -y python3
+        pip install flake8 pyright black pyflakes
+        if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
     - name: Install cfbs
       run: pip install cfbs
     - name: Check the status with cfbs
@@ -22,3 +29,16 @@ jobs:
       run: cfbs validate
     - name: Check the formatting
       run: cfbs --check pretty ./cfbs.json
+    - name: Lint with flake8
+      run: |
+        flake8 . --ignore=E203,W503,E722,E731 --max-complexity=100 --max-line-length=160
+    - name: Lint with pyright (type checking)
+      run: |
+        pyright .
+    - name: Check formatting with black
+      run: |
+        shopt -s globstar
+        black --check .
+    - name: Run pyflakes
+      run: |
+        pyflakes .

examples/git-using-lib/git_using_lib.py

@@ -10,7 +10,7 @@ def validate_promise(self, promiser, attributes, metadata):
         if not promiser.startswith("/"):
             raise ValidationError(f"File path '{promiser}' must be absolute")
         if "repository" not in attributes:
-            raise ValidationError(f"Attribute 'repository' is required")
+            raise ValidationError("Attribute 'repository' is required")
 
     def evaluate_promise(self, promiser, attributes, metadata):
         url = attributes["repository"]

examples/gpg/gpg.py

@@ -42,8 +42,7 @@
 """
 
 import json
-from subprocess import Popen, PIPE
-import sys
+from subprocess import Popen, PIPE, TimeoutExpired
 from cfengine_module_library import PromiseModule, ValidationError, Result
 
 
@@ -109,9 +108,9 @@ def validate_promise(self, promiser, attributes, metadata):
             raise ValidationError(
                 f"Promiser '{promiser}' for 'gpg_keys' promise must be an absolute path"
             )
-        if not "keylist" in attributes:
+        if "keylist" not in attributes:
             raise ValidationError(
-                f"Required attribute 'keylist' missing for 'gpg_keys' promise"
+                "Required attribute 'keylist' missing for 'gpg_keys' promise"
             )
 
     def evaluate_promise(self, promiser, attributes, metadata):

examples/rss/rss.py

@@ -1,4 +1,7 @@
-import requests, html, re, os, random
+import requests
+import re
+import os
+import random
 import xml.etree.ElementTree as ET
 from cfengine_module_library import PromiseModule, ValidationError, Result
 
@@ -42,7 +45,7 @@ def validate_promise(self, promiser, attributes, metadata):
             # check that attribute select has a valid type
             if type(select) is not str:
                 raise ValidationError(
-                    f"Invalid type for attribute select: expected string"
+                    "Invalid type for attribute select: expected string"
                 )
 
             # check that attribute select has a valid value
@@ -159,18 +162,18 @@ def _write_promiser(self, item, promiser):
             return Result.NOT_KEPT
 
     def _is_win_file(self, path):
-        return re.search(r"^[a-zA-Z]:\\[\\\S|*\S]?.*$", path) != None
+        return re.search(r"^[a-zA-Z]:\\[\\\S|*\S]?.*$", path) is not None
 
     def _is_unix_file(self, path):
-        return re.search(r"^(/[^/ ]*)+/?$", path) != None
+        return re.search(r"^(/[^/ ]*)+/?$", path) is not None
 
     def _is_url(self, path):
         return (
             re.search(
                 r"^http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+",
                 path,
             )
-            != None
+            is not None
         )
 
 

examples/site-up/site_up.py

@@ -31,7 +31,7 @@ def evaluate_promise(self, promiser, attributes, metadata):
 
         error = None
         try:
-            code = urllib.request.urlopen(url, context=ssl_ctx).getcode()
+            urllib.request.urlopen(url, context=ssl_ctx).getcode()
             self.log_verbose(f"Site '{url}' is UP!")
             return Result.KEPT
         except urllib.error.HTTPError as e:

libraries/python/cfengine_module_library.py

@@ -226,6 +226,8 @@ def _handle_request(self, request):
             "debug",
         ]
 
+        promiser = None
+        attributes = {}
         if operation in ["validate_promise", "evaluate_promise"]:
             promiser = request["promiser"]
             attributes = request.get("attributes", {})
@@ -404,7 +406,7 @@ def _handle_evaluate(self, promiser, attributes, request):
             assert results is not None  # Most likely someone forgot to return something
 
             # evaluate_promise should return either a result or a (result, result_classes) pair
-            if type(results) == str:
+            if isinstance(results, str):
                 self._result = results
             else:
                 assert len(results) == 2

promise-types/ansible/ansible_promise.py

@@ -1,74 +1,64 @@
 import os
 
 from typing import Dict, Tuple, List
-
 from cfengine_module_library import PromiseModule, ValidationError, Result
 
-try:
-    from ansible import context
-    from ansible.cli import CLI
-    from ansible.executor.playbook_executor import PlaybookExecutor
-    from ansible.inventory.manager import InventoryManager
-    from ansible.module_utils.common.collections import ImmutableDict
-    from ansible.parsing.dataloader import DataLoader
-    from ansible.plugins.callback import CallbackBase
-    from ansible.vars.manager import VariableManager
-    from ansible.plugins.loader import init_plugin_loader
-
-    ANSIBLE_AVAILABLE = True
-except ImportError:
-    ANSIBLE_AVAILABLE = False
-
-
-if ANSIBLE_AVAILABLE:
-
-    class CallbackModule(CallbackBase):
-        CALLBACK_VERSION = 1.0
-        CALLBACK_TYPE = "stdout"
-        CALLBACK_NAME = "cfengine"
-
-        def __init__(self, *args, promise=None, **kw):
-            self.promise = promise
-            self.hosts = set()
-            self.changed = False
-            super(CallbackModule, self).__init__(*args, **kw)
-
-        def v2_runner_on_start(self, host, task):
-            self.hosts.add(str(host))
-            self.promise.log_verbose(
-                "Task '" + task.name + "' started on '" + str(host) + "'"
-            )
+import ansible.context as context
+from ansible.cli import CLI
+from ansible.executor.playbook_executor import PlaybookExecutor
+from ansible.inventory.manager import InventoryManager
+from ansible.module_utils.common.collections import ImmutableDict
+from ansible.parsing.dataloader import DataLoader
+from ansible.plugins.callback import CallbackBase
+from ansible.vars.manager import VariableManager
+from ansible.plugins.loader import init_plugin_loader
+
+
+class CallbackModule(CallbackBase):
+    CALLBACK_VERSION = 1.0
+    CALLBACK_TYPE = "stdout"
+    CALLBACK_NAME = "cfengine"
+
+    def __init__(self, *args, promise, **kw):
+        self.promise = promise
+        self.hosts = set()
+        self.changed = False
+        super(CallbackModule, self).__init__(*args, **kw)
+
+    def v2_runner_on_start(self, host, task):
+        self.hosts.add(str(host))
+        self.promise.log_verbose(
+            "Task '" + task.name + "' started on '" + str(host) + "'"
+        )
 
-        def v2_runner_on_ok(self, result):
-            is_changed = result.is_changed()
-            if is_changed:
-                self.changed = True
-                self.promise.log_info(
-                    "Task '" + result.task_name + "' successfully changed"
-                )
-            else:
-                self.promise.log_verbose(
-                    "Task '" + result.task_name + "' didn't change"
-                )
+    def v2_runner_on_ok(self, result):
+        is_changed = result.is_changed()
+        if is_changed:
+            self.changed = True
+            self.promise.log_info(
+                "Task '" + result.task_name + "' successfully changed"
+            )
+        else:
+            self.promise.log_verbose("Task '" + result.task_name + "' didn't change")
 
-        def v2_runner_on_failed(self, result, **_):
-            self.promise.log_error("Task '" + result.task_name + "' failed")
+    def v2_runner_on_failed(self, result, ignore_errors=False):
+        self.promise.log_error("Task '" + result.task_name + "' failed")
 
-        def v2_runner_on_skipped(self, result):
-            self.promise.log_error("Task '" + result.task_name + "' was skipped")
+    def v2_runner_on_skipped(self, result):
+        self.promise.log_error("Task '" + result.task_name + "' was skipped")
 
-        def v2_playbook_on_stats(self, stats):
-            for host in self.hosts:
-                summary_dict = stats.summarize(host)
-                summary = " ".join(
-                    "%s=%s" % (k, v) for k, v in summary_dict.items() if v > 0
+    def v2_playbook_on_stats(self, stats):
+        for host in self.hosts:
+            summary_dict = stats.summarize(host)
+            summary = " ".join(
+                "%s=%s" % (k, v) for k, v in summary_dict.items() if v > 0
+            )
+            if summary_dict.get("unreachable"):
+                self.promise.log_error("Host '" + host + "' is unreachable")
+            elif summary:
+                self.promise.log_verbose(
+                    "Summary of the tasks for '" + host + "' is: " + summary
                 )
-                if summary_dict.get("unreachable"):
-                    self.promise.log_error("Host '" + host + "' is unreachable")
-                else:
-                    summary and self.promise.log_verbose(
-                        "Summary of the tasks for '" + host + "' is: " + summary
-                    )
 
 
 class AnsiblePromiseTypeModule(PromiseModule):
@@ -100,8 +90,7 @@ def prepare_promiser_and_attributes(self, promiser, attributes):
         return (safe_promiser, attributes)
 
     def validate_promise(self, promiser: str, attributes: Dict, metadata: Dict):
-        if not ANSIBLE_AVAILABLE:
-            raise ValidationError("Ansible Python module not available")
+        return
 
     def evaluate_promise(
         self, safe_promiser: str, attributes: Dict, metadata: Dict
@@ -153,7 +142,7 @@ def evaluate_promise(
             passwords={},
         )
         callback = CallbackModule(promise=self)
-        pbex._tqm._stdout_callback = callback
+        pbex._tqm._stdout_callback = callback  # type: ignore
 
         exit_code = pbex.run()
         if exit_code != 0:

promise-types/git/git.py

@@ -4,7 +4,12 @@
 
 from typing import Dict, List, Optional
 
-from cfengine_module_library import PromiseModule, ValidationError, Result
+from cfengine_module_library import (
+    PromiseModule,
+    ValidationError,
+    Result,
+    AttributeObject,
+)
 
 
 class GitPromiseTypeModule(PromiseModule):
@@ -93,7 +98,8 @@ def evaluate_promise(self, promiser: str, attributes: Dict, metadata: Dict):
                 result = Result.REPAIRED
             except subprocess.CalledProcessError as e:
                 self.log_error("Failed clone: {error}".format(error=e.output or e))
-                e.stderr and self.log_error(e.stderr.strip())
+                if e.stderr:
+                    self.log_error(e.stderr.strip())
                 return (
                     Result.NOT_KEPT,
                     [
@@ -134,7 +140,8 @@ def evaluate_promise(self, promiser: str, attributes: Dict, metadata: Dict):
                         result = Result.REPAIRED
                 except subprocess.CalledProcessError as e:
                     self.log_error("Failed reset: {error}".format(error=e.output or e))
-                    e.stderr and self.log_error(e.stderr.strip())
+                    if e.stderr:
+                        self.log_error(e.stderr.strip())
                     return (
                         Result.NOT_KEPT,
                         [
@@ -228,7 +235,8 @@ def evaluate_promise(self, promiser: str, attributes: Dict, metadata: Dict):
                     )
                 except subprocess.CalledProcessError as e:
                     self.log_error("Failed fetch: {error}".format(error=e.output or e))
-                    e.stderr and self.log_error(e.stderr.strip())
+                    if e.stderr:
+                        self.log_error(e.stderr.strip())
                     return (
                         Result.NOT_KEPT,
                         [
@@ -241,7 +249,9 @@ def evaluate_promise(self, promiser: str, attributes: Dict, metadata: Dict):
         # everything okay
         return (result, classes)
 
-    def _git(self, model: object, args: List[str], cwd: Optional[str] = None) -> str:
+    def _git(
+        self, model: AttributeObject, args: List[str], cwd: Optional[str] = None
+    ) -> str:
         self.log_verbose("Run: {cmd}".format(cmd=" ".join(args)))
         output = (
             subprocess.check_output(
@@ -253,15 +263,16 @@ def _git(self, model: object, args: List[str], cwd: Optional[str] = None) -> str
             .strip()
             .decode("utf-8")
         )
-        output != "" and self.log_verbose(output)
+        if output != "":
+            self.log_verbose(output)
         return output
 
-    def _git_envvars(self, model: object):
+    def _git_envvars(self, model: AttributeObject):
         env = os.environ.copy()
         env["GIT_SSH_COMMAND"] = model.ssh_executable
         if model.ssh_options:
             env["GIT_SSH_COMMAND"] += " " + model.ssh_options
-        if not "HOME" in env:
+        if "HOME" not in env:
             # git should have a HOME env var to retrieve .gitconfig, .git-credentials, etc
             env["HOME"] = str(Path.home())
         return env

promise-types/groups/groups.py

@@ -64,7 +64,7 @@ def validate_promise(self, promiser, attributes, metadata):
                 )
 
             # check attribute gid value
-            if type(gid) == str:
+            if isinstance(gid, str):
                 try:
                     int(gid)
                 except ValueError: