Python3 deploy action mypy to review this pr

.github/workflows/main.yml → .github/main.yml

@@ -14,16 +14,18 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        docker-image:  # See https://hub.docker.com/_/python/tags for images
-          - python:2.7.18-alpine3.11
-          - python:3.11.4-alpine3.18
-    container: ${{ matrix.docker-image }}
+        python-version: ['2.7', '3.11']
     steps:
       - name: Checkout code
         uses: actions/checkout@v3
 
+      - name: Setup Python ${{matrix.python-version}} (without a container)
+        uses: LizardByte/setup-python-action@master
+        with:
+          python-version: ${{matrix.python-version}}
+
       - name: Install python 2 dependencies
-        if: ${{ startsWith(matrix.docker-image, 'python:2.7.18') }}
+        if: ${{ matrix.python-version == '2.7' }}
         run: pip install enum
 
       - name: Install dependencies
@@ -115,3 +117,45 @@ jobs:
 
       - name: quality-gate
         run: make quality-gate
+
+      - uses: reviewdog/action-actionlint@v1
+        name: GitHub Action linter from https://github.com/reviewdog/action-actionlint
+        with:
+          github_token: ${{ secrets.github_token }}
+          level: error
+          reporter: github-pr-review
+
+      - uses: tsuyoshicho/action-mypy@v3.13.0
+        with:
+          filter_mode: file
+          setup_method: install
+          setup_command: pip install mypy mock
+          mypy_flags: |
+            --scripts-are-modules
+            --check-untyped-defs
+            --allow-redefinition
+            --ignore-missing-imports
+            --no-pretty
+            --warn-unreachable
+            --exclude ocaml/
+            --exclude scripts/examples
+            --exclude scripts/examples/python/monitor-unwanted-domains.py
+            --exclude scripts/scalability-tests/ping-master.py
+            --exclude scripts/backup-sr-metadata.py
+            --exclude scripts/restore-sr-metadata.py
+            --exclude scripts/nbd_client_manager.py
+            --config-file .mypy.ini
+          target: |
+            scripts/perfmon
+            scripts/hfx_filename
+            scripts/mail-alarm
+            scripts/host-display
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review
+
+      - uses: dciborow/action-pylint@0.1.1
+        with:
+          filter_mode: file
+          glob_pattern: "scripts/*"
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review

.github/workflows/python-code-review.yml

@@ -0,0 +1,59 @@
+name: Python Code Review
+on: [pull_request]
+
+# Cancel a currently running workflow from the same PR, branch or tag when
+# a new workflow is triggered:
+# https://stackoverflow.com/questions/66335225/how-to-cancel-previous-runs-in-the-pr-when-you-push-new-commitsupdate-the-curre
+concurrency:
+  cancel-in-progress: true
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+
+jobs:
+  python-code-review:
+    name: Python Code Review
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - uses: tsuyoshicho/action-mypy@v3.13.0
+        with:
+          filter_mode: file
+          setup_method: install
+          setup_command: pip install mypy mock
+          mypy_flags: |
+            --scripts-are-modules
+            --check-untyped-defs
+            --allow-redefinition
+            --ignore-missing-imports
+            --no-pretty
+            --warn-unreachable
+            --exclude ocaml/
+            --exclude scripts/examples
+            --exclude scripts/examples/python/monitor-unwanted-domains.py
+            --exclude scripts/scalability-tests/ping-master.py
+            --exclude scripts/backup-sr-metadata.py
+            --exclude scripts/restore-sr-metadata.py
+            --exclude scripts/nbd_client_manager.py
+            --config-file .mypy.ini
+          target: |
+            scripts/perfmon
+            scripts/hfx_filename
+            scripts/mail-alarm
+            scripts/host-display
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review
+
+      - uses: dciborow/action-pylint@0.1.1
+        with:
+          filter_mode: file
+          glob_pattern: "scripts/*"
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review
+
+      - uses: reviewdog/action-actionlint@v1
+        name: GitHub Action linter from https://github.com/reviewdog/action-actionlint
+        with:
+          github_token: ${{ secrets.github_token }}
+          level: error
+          reporter: github-pr-review

.mypy.ini

@@ -0,0 +1,48 @@
+[mypy-XenAPI.*]
+# TODO/FIXME:
+disable_error_code = union-attr
+[mypy]
+# TODO/FIXME:
+exclude = (?x)(
+    ^ocaml/
+    | scripts/.*usb_scan.py
+    | scripts/examples
+    | scripts/examples/python/monitor-unwanted-domains.py
+    | scripts/scalability-tests/ping-master.py
+    | scripts/backup-sr-metadata.py
+    | scripts/restore-sr-metadata.py
+    | scripts/nbd_client_manager.py
+  )
+# Python scripts that don't end in .py must be given here:
+files =
+# TODO/FIXME: Test more scripts by default after those are fixed:
+    scripts/hfx_filename,
+    scripts/perfmon,
+    scripts/mail-alarm,
+    scripts/host-display,
+# TODO/FIXME:
+disable_error_code =
+    attr-defined,
+    import-not-found,
+    import-untyped,
+    type-arg,
+    var-annotated,
+mypy_path = scripts/examples/python:.:scripts:scripts/plugins:scripts/examples
+pretty = true
+error_summary = true
+strict_equality = true
+show_error_codes = true
+show_error_context = true
+# Check the contents of untyped functions in all modules by default:
+check_untyped_defs = true
+scripts_are_modules = true
+# TODO/FIXME: mypy does not support targetting 3.6 anymore:
+python_version = 3.8
+warn_return_any = true
+warn_unreachable = true
+warn_unused_configs = true
+warn_redundant_casts = true
+disallow_any_explicit = false
+disallow_any_generics = true
+disallow_any_unimported = true
+disallow_subclassing_any = true

.pylintrc

@@ -0,0 +1,75 @@
+[MAIN]
+
+# Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the
+# number of processors available to use, and will cap the count on Windows to
+# avoid hangs.
+jobs=2
+
+# Pickle collected data for later comparisons.
+persistent=yes
+
+# Discover python modules and packages in the file system subtree.
+recursive=yes
+
+# Add paths to the list of the source roots. Supports globbing patterns. The
+# source root is an absolute path or a path relative to the current working
+# directory used to determine a package namespace for modules located under the
+# source root.
+source-roots=
+    ocaml/xapi-storage/python,
+    scripts,
+    scripts/examples/python,
+
+# When enabled, pylint would attempt to guess common misconfiguration and emit
+# user-friendly hints instead of false-positive error messages.
+suggestion-mode=yes
+
+# In verbose mode, extra non-checker-related info will be displayed.
+verbose=yes
+
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then re-enable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=
+    bad-option-value, # old pylint for py2: ignore newer (unknown) pylint options
+    bad-continuation, # old pylint warns about some modern black formatting
+    bad-indentation,
+    consider-using-dict-comprehension,
+    consider-using-f-string,
+    consider-using-with,
+    import-error,
+    import-outside-toplevel,
+    invalid-name,
+    missing-final-newline,
+    missing-function-docstring,
+    multiple-imports,
+    redefined-outer-name,
+    trailing-whitespace,  # enable this soon, after citical PRs are merged
+    useless-option-value, # new pylint has abaondoned these old options
+
+[FORMAT]
+
+# Maximum number of characters on a single line.
+max-line-length=98
+
+[BASIC]
+
+# Good variable names which should always be accepted, separated by a comma.
+good-names=a,
+           b,
+           c,
+           f,
+           i,
+           j,
+           k,
+           ex,
+           Run,
+           _

scripts/hfx_filename

@@ -1,4 +1,4 @@
 #!/usr/bin/env python3
 
 # Copyright (c) 2015 Citrix, Inc.
 #
@@ -28,23 +28,19 @@
         headers = [
           "POST %s?session_id=%s HTTP/1.0" % (db_url, session_id),
           "Connection:close",
-          "content-length:%d" % (len(request)),
+          "content-length:%d" % (len(request.encode('utf-8'))),
           ""
         ]
-        #print "Sending HTTP request:"
         for h in headers:
-          s.send("%s\r\n" % h)
-          #print "%s\r\n" % h,
-        s.send(request)
+          s.send((h + "\r\n").encode('utf-8'))
+        s.send(request.encode('utf-8'))
 
-        result = s.recv(1024)
-        #print "Received HTTP response:"
-        #print result
+        result = s.recv(1024).decode('utf-8')
         if "200 OK" not in result:
           print("Expected an HTTP 200, got %s" % result, file=sys.stderr)
           return
         id = result.find("\r\n\r\n")
         headers = result[:id]
         for line in headers.split("\r\n"):
           cl = "content-length:"
           if line.lower().startswith(cl):
@@ -57,11 +53,11 @@
 def parse_string(txt):
     prefix = "<value><array><data><value>success</value><value>"
     if not txt.startswith(prefix):
-        raise "Unable to parse string response"
+        raise Exception("Unable to parse string response")
     txt = txt[len(prefix):]
     suffix = "</value></data></array></value>"
     if not txt.endswith(suffix):
-        raise "Unable to parse string response"
+        raise Exception("Unable to parse string response")
     txt = txt[:len(txt)-len(suffix)]
     return txt
 

scripts/import_file.py

@@ -0,0 +1,24 @@
+# Used for importing a non-".py" file as a module
+
+import sys
+import os
+
+def import_from_file(module_name, file_path):
+    """Import a file as a module"""
+    # Only for python3, but CI has python2 pytest check, so add this line
+    if sys.version_info.major == 2:
+        return None
+    from importlib import machinery, util
+    loader = machinery.SourceFileLoader(module_name, file_path)
+    spec = util.spec_from_loader(module_name, loader)
+    assert spec
+    assert spec.loader
+    module = util.module_from_spec(spec)
+    # Probably a good idea to add manually imported module stored in sys.modules
+    sys.modules[module_name] = module
+    spec.loader.exec_module(module)
+    return module
+
+def get_module(module_name, file_path):
+    testdir = os.path.dirname(__file__)
+    return import_from_file(module_name, testdir + file_path)

scripts/perfmon

@@ -1,4 +1,4 @@
 #!/usr/bin/env python3
 #
 # perfmon - a daemon for monitoring performance of the host on which it is run 
 # and of all the local VMs, and for generating events based on configurable 
@@ -295,10 +295,10 @@
         # params are what get passed to the CGI executable in the URL
         self.params = dict()
         self.params['start'] = int(time.time()) - interval # interval seconds ago
         self.params['host'] = 'true'   # include data for host (as well as for VMs)
         self.params['sr_uuid'] = 'all' # include data for all SRs attached to this host
         self.params['cf'] = 'AVERAGE'  # consolidation function, each sample averages 12 from the 5 second RRD
         self.params['interval'] = str(rrd_step) # distinct from the perfmon interval
         self.report = RRDReport()      # data structure updated by RRDContentHandler
 
     def __repr__(self):
@@ -313,7 +313,7 @@
         print_debug("Calling http://localhost/rrd_updates?%s" % paramstr)
 
         sock = urllib.request.urlopen("http://localhost/rrd_updates?%s" % paramstr)
-        xmlsource = sock.read()
+        xmlsource = sock.read().decode('utf-8')
         sock.close()
 
         # Use sax rather than minidom and save Vvvast amounts of time and memory.
@@ -380,16 +380,16 @@
         memlist = memfd.readlines()
         memfd.close()
         memdict = [ m.split(':', 1) for m in memlist ]
-        memdict = dict([(k.strip(), float(re.search('\d+', v.strip()).group(0))) for (k,v) in memdict])
+        memdict = dict([(k.strip(), float(re.search(r'\d+', v.strip()).group(0))) for (k,v) in memdict])
         # We consider the sum of res memory and swap in use as the hard demand
         # of mem usage, it is bad if this number is beyond the physical mem, as
         # in such case swapping is obligatory rather than voluntary, hence
         # degrading the performance. We define the percentage metrics as
         # (res_mem + swap_in_use) / phy_mem, which could potentially go beyond
         # 100% (but is considered bad when it does)
         mem_in_use = memdict['MemTotal'] - memdict['MemFree'] - memdict['Buffers'] - memdict['Cached']
         swap_in_use = memdict['SwapTotal'] - memdict['SwapFree']
         return float(mem_in_use + swap_in_use) / memdict['MemTotal']
     except Exception as e:
         log_err("Error %s in get_percent_mem_usage, return 0.0 instead" % e)
         return 0.0
@@ -514,7 +514,7 @@
         if delta < self.alarm_auto_inhibit_period:
             return # we are in the auto inhibit period - do nothing
         self.timeof_last_alarm = t
         message = "value: %f\nconfig:\n%s" % (self.value, self.xmldoc.toprettyxml())
 
         self.alarm_create_callback(self, session, message)
 
@@ -1067,7 +1067,8 @@
             vm_uuid_list = rrd_updates.get_uuid_list_by_objtype('vm')
 
             # Remove any monitors for VMs no longer listed in rrd_updates page
-            for uuid in vm_mon_lookup:
+            # We use .pop() inside the loop, use list(dict_var.keys()):
+            for uuid in list(vm_mon_lookup.keys()):
                 if uuid not in vm_uuid_list:
                     vm_mon_lookup.pop(uuid)
 
@@ -1091,7 +1092,7 @@
                 host_mon = None
             elif not host_mon:
                 host_mon = HOSTMonitor(host_uuid)
             elif host_mon.uuid != host_uuid:
                 raise PerfMonException("host uuid in rrd_updates changed (old: %s, new %s)" % \
                     (host_mon.uuid, host_uuid))
             else:
@@ -1103,7 +1104,8 @@
             print_debug("sr_uuid_list = %s" % sr_uuid_list)
 
             # Remove monitors for SRs no longer listed in the rrd_updates page
-            for uuid in sr_mon_lookup:
+            # We use .pop() inside the loop, use list(dict_var.keys()):
+            for uuid in list(sr_mon_lookup.keys()):
                 if uuid not in sr_uuid_list:
                     sr_mon_lookup.pop(uuid)
             # Create monitors for SRs that have just appeared in rrd_updates page