Merge pull request #1614 from chiragkyal/fix-py-lint

USHIFT-1030: fix pylint finding issues

Makefile

@@ -116,7 +116,7 @@ etcd:
 		$(MAKE) -C etcd
 
 .PHONY: verify verify-images verify-assets licensecheck
-verify: verify-images verify-assets verify-sh verify-container licensecheck
+verify: verify-images verify-assets verify-sh verify-py verify-container licensecheck
 
 verify-images:
 	./hack/verify_images.sh
@@ -147,10 +147,7 @@ verify-sh:
 
 .PHONY: verify-py
 verify-py:
-	@if ! command -v pylint &>/dev/null; then \
-		pip install pylint ; \
-	fi
-	pylint $$(find . -type d \( -path ./_output -o -path ./vendor -o -path ./assets -o -path ./etcd/vendor \) -prune -o -name '*.py' -print)
+	./hack/verify-py.sh
 
 .PHONY: verify-container
 verify-container:

hack/verify-py.sh

@@ -0,0 +1,37 @@
+#!/bin/bash
+
+set -euo pipefail
+
+ROOTDIR=$(git rev-parse --show-toplevel)
+VENV_DIR="${ROOTDIR}/_output"
+VENV="${VENV_DIR}/venv"
+
+REQ_FILE=${ROOTDIR}/scripts/requirements.txt
+
+create_venv() {
+    local vpython="${VENV}/bin/python3"
+
+    [ -f "${REQ_FILE}" ] || { echo "${REQ_FILE} is not present"; exit 1; }
+    [ -d "${VENV_DIR}" ] || mkdir -p "${VENV_DIR}"
+
+    echo "Creating venv in '${VENV}' and installing packages..."
+    python3 -m venv ${VENV}
+    ${vpython} -m pip install --upgrade pip
+    ${vpython} -m pip install -r ${REQ_FILE}
+    echo "Done!"
+}
+
+run_pylint() {
+    local pylint="${VENV}/bin/pylint"
+    local pyfiles=$(find . -type d \( -path ./_output -o -path ./vendor -o -path ./assets -o -path ./etcd/vendor \) -prune -o -name '*.py' -print)
+
+    if ! command -v ${pylint} &>/dev/null; then
+        echo "Installing pylint..."
+        create_venv
+    fi
+
+    printf "Running ${pylint} for \n${pyfiles}\n"
+    ${pylint} --variable-naming-style=any ${pyfiles}
+}
+
+run_pylint

scripts/auto-rebase/handle-assets.py → scripts/auto-rebase/handle_assets.py

@@ -1,16 +1,24 @@
 #!/usr/bin/env python3
+# pylint: disable=logging-fstring-interpolation,logging-not-lazy,line-too-long
 
+"""
+This script updates assets based on a YAML recipe (`assets.yaml`)
+The recipe specifies what files and directories should be copied, ignored, and restored.
+
+File: handle_assets.py
+"""
+
+import logging
 import os
 import sys
-import yaml
 import shutil
-import logging
 import subprocess
+import yaml
 
 try:
-    from yaml import CLoader as Loader, CDumper as Dumper
+    from yaml import CLoader as Loader
 except ImportError:
-    from yaml import Loader, Dumper
+    from yaml import Loader
 
 logging.basicConfig(level=logging.DEBUG, format='%(asctime)-27s %(levelname)-10s %(message)s')
 
@@ -19,18 +27,27 @@
 
 
 def merge_paths(pathl, pathr):
+    """
+    Merge two paths depending upon following conditions:
+    - If `pathr` is absolute (starts with `/`), then discard the leading `/` and return rest `pathr`.
+    - If `pathr` is relative, then return `pathl/pathr`.
+    """
     if pathr.startswith("/"):
         return pathr[1:]
     return os.path.join(pathl, pathr)
 
 
-def run_command(args=[]):
+def run_command(args=None):
+    """Run a command with the given args and return True if successful."""
+    if args is None:
+        args = []
     if not args:
         logging.error("run_command() received empty args")
         sys.exit(1)
 
     logging.debug(f"Executing '{' '.join(args)}'")
-    result = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, universal_newlines=True)
+    result = subprocess.run(
+        args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, universal_newlines=True,check=False)
 
     if result.returncode != 0:
         logging.error(f"Command '{' '.join(args)}' returned {result.returncode}. Output: {result.stdout}")
@@ -40,26 +57,30 @@ def run_command(args=[]):
 
 
 def git_restore(path):
+    """Restore a file from git."""
     path = os.path.join(ASSETS_DIR, path)
     logging.info(f"Restoring {path}")
     return run_command(["git", "restore", path])
 
 
 def copy(src, dst):
+    """Copy a file from the source path to the destination path."""
     src = os.path.join(STAGING_DIR, src)
     dst = os.path.join(ASSETS_DIR, dst)
     logging.debug(f"Copying {dst} <- {src}")
     shutil.copyfile(src, dst)
 
 
 def clear_dir(path):
+    """Clear the contents of a directory."""
     path = os.path.join(ASSETS_DIR, path)
     logging.info(f"Clearing directory {path}")
     shutil.rmtree(path)
     os.makedirs(path)
 
 
 def should_be_ignored(asset, dst):
+    """Check if an asset should be ignored based on its 'ignore' field."""
     if 'ignore' in asset:
         reason = asset['ignore']
         if not reason:
@@ -71,6 +92,7 @@ def should_be_ignored(asset, dst):
 
 
 def handle_file(file, dst_dir="", src_prefix=""):
+    """Handle a file by copying, restoring or ignoring it."""
     name = file['file']
     dst = merge_paths(dst_dir, name)
 
@@ -89,26 +111,28 @@ def handle_file(file, dst_dir="", src_prefix=""):
     copy(src, dst)
 
 
-def handle_dir(dir, dst_dir="", src_prefix=""):
-    dst = merge_paths(dst_dir, dir['dir'])
-    new_src_prefix = merge_paths(src_prefix, dir['src'] if "src" in dir else "")
+def handle_dir(dir_, dst_dir="", src_prefix=""):
+    """"Recursively handle a directory, its files and subdirectories."""
+    dst = merge_paths(dst_dir, dir_['dir'])
+    new_src_prefix = merge_paths(src_prefix, dir_['src'] if "src" in dir_ else "")
 
-    if should_be_ignored(dir, dst):
+    if should_be_ignored(dir_, dst):
         return
 
-    if dir.get('no_clean', False):
+    if dir_.get('no_clean', False):
         logging.info(f"Not clearing dir {dst}")
     else:
         clear_dir(dst)
 
-    for f in dir.get('files', []):
-        handle_file(f, dst, new_src_prefix)
+    for file in dir_.get('files', []):
+        handle_file(file, dst, new_src_prefix)
 
-    for d in dir.get('dirs', []):
-        handle_dir(d, dst, new_src_prefix)
+    for sub_dir in dir_.get('dirs', []):
+        handle_dir(sub_dir, dst, new_src_prefix)
 
 
 def main():
+    """Main function for handling assets based on the recipe file."""
     if not os.path.isdir(ASSETS_DIR):
         logging.error(f"Expected to run in root directory of microshift repository but was in {os.getcwd()}")
         sys.exit(1)
@@ -118,7 +142,9 @@ def main():
         sys.exit(1)
 
     recipe_filepath = "./scripts/auto-rebase/assets.yaml"
-    recipe = yaml.load(open(recipe_filepath).read(), Loader=Loader)
+    with open(recipe_filepath, encoding='utf-8') as recipe_file:
+        recipe = yaml.load(recipe_file.read(), Loader=Loader)
+
     for asset in recipe['assets']:
         if "dir" in asset:
             handle_dir(asset)

scripts/auto-rebase/presubmit.py

@@ -1,11 +1,20 @@
 #!/usr/bin/env python3
+# pylint: disable=line-too-long
 
+"""
+This script verifies all the assets for auto-rebase.
+It checks that all files in the assets directory are listed in the assets.yaml file, and vice versa.
+
+File: presubmit.py
+"""
+
+from functools import reduce
+import glob
 import os
 import sys
-import glob
 import yaml
-from functools import reduce
 
+# pylint: disable=R0801
 try:
     from yaml import CLoader as Loader
 except ImportError:
@@ -16,29 +25,33 @@
 RECIPE_FILEPATH = "./scripts/auto-rebase/assets.yaml"
 
 
-def build_assets_filelist_from_asset_dir(dir, prefix=""):
-    dir_path = os.path.join(prefix, dir['dir'])
-    return ([os.path.join(dir_path, f['file']) for f in dir.get('files', [])] +
+def build_assets_filelist_from_asset_dir(asset_dir, prefix=""):
+    """Recursively builds a list of assets filepaths from an asset directory."""
+    dir_path = os.path.join(prefix, asset_dir['dir'])
+    return ([os.path.join(dir_path, f['file']) for f in asset_dir.get('files', [])] +
             reduce(lambda x, y: x+y,
-                   [build_assets_filelist_from_asset_dir(subdir, dir_path) for subdir in dir.get('dirs', [])],
+                   [build_assets_filelist_from_asset_dir(subdir, dir_path) for subdir in asset_dir.get('dirs', [])],
                    []))
 
 
 def build_assets_filelist_from_recipe(recipe):
-    return reduce(lambda x, y: x+[y] if type(y) == str else x+y,
+    """Builds a list of assets filepaths from a recipe file."""
+    return reduce(lambda x, y: x+[y] if isinstance(y, str) else x+y,
                   [build_assets_filelist_from_asset_dir(asset) if 'dir' in asset else asset['file'] for asset in recipe['assets']],
                   [])
 
 
 def main():
+    """Main function for checking assets against an asset recipe."""
     if not os.path.isdir(ASSETS_DIR):
         print(f"ERROR: Expected to run in root directory of microshift repository but was in {os.getcwd()}")
         sys.exit(1)
 
-    recipe = yaml.load(open(RECIPE_FILEPATH).read(), Loader=Loader)
+    with open(RECIPE_FILEPATH, encoding='utf-8') as recipe_file:
+        recipe = yaml.load(recipe_file.read(), Loader=Loader)
 
     assets_filelist = set(build_assets_filelist_from_recipe(recipe))
-    realfiles = set([f.replace('assets/', '') for f in glob.glob('assets/**/*.*', recursive=True)])
+    realfiles = {f.replace('assets/', '') for f in glob.glob('assets/**/*.*', recursive=True)}
 
     missing_in_recipe = realfiles - assets_filelist
     superfluous_in_recipe = assets_filelist - realfiles