Fix various dev tool install issues, make shellcheck a required lint (#…

…1047)

Signed-off-by: Kevin M Granger <[email protected]>
Co-authored-by: Mateus Oliveira <[email protected]>

.github/workflows/shellcheck.yml

@@ -16,10 +16,6 @@ on:
 jobs:
   unit-test:
     runs-on: ubuntu-latest
-    strategy:
-      fail-fast: false
-      matrix:
-        python-version: ['3.10', '3.11']
 
     steps:
       - name: Checkout
@@ -28,7 +24,7 @@ jobs:
       - name: Set up Python ${{ matrix.python-version }}
         uses: actions/setup-python@v4
         with:
-          python-version: ${{ matrix.python-version }}
+          python-version: "3.11"
           cache: 'pip'
           cache-dependency-path: |
             **/requirements*.txt

Makefile

@@ -185,9 +185,9 @@ isort-check: $(PELORUS_VENV)
 
 # Linting
 
-.PHONY: lint python-lint pylava chart-lint chart-lint-optional shellcheck shellcheck-optional typecheck
+.PHONY: lint python-lint pylava chart-lint chart-lint-optional shellcheck typecheck
 ## lint: lint python code, shell scripts, and helm charts
-lint: python-lint chart-lint-optional shellcheck-optional
+lint: python-lint chart-lint-optional shellcheck
 
 ## python-lint: lint python files
 python-lint: $(PELORUS_VENV)
@@ -221,17 +221,10 @@ chart-lint-optional:
 endif
 
 shellcheck: $(PELORUS_VENV) $(PELORUS_VENV)/bin/shellcheck
+	@echo "🐚 📋 Linting shell scripts with shellcheck"
 	. ${PELORUS_VENV}/bin/activate && \
-	if [[ -z shellcheck ]]; then echo "Shellcheck is not installed" >&2; false; fi && \
-	echo "🐚 📋 Linting shell scripts with shellcheck" && \
 	shellcheck $(shell find . -name '*.sh' -type f | grep -v 'venv/\|git/\|.pytest_cache/\|htmlcov/\|_test/test_helper/\|_test/bats\|_test/conftest')
 
-ifneq (, $(SHELLCHECK))
-shellcheck-optional: shellcheck
-else
-shellcheck-optional:
-	$(warning 🐚 ⏭ Shellcheck not found, skipping)
-endif
 
 ## doc-check: Check if there is any problem with the project documentation generation
 doc-check: $(PELORUS_VENV)

scripts/get_tool_dl_url.py

@@ -73,7 +73,7 @@ class Nooba:
     "Nooba's URL pattern."
 
     repo = "noobaa/noobaa-operator"
-    arch = "mac" if OS == "Darwin" else "linux"
+    arch = OS.lower()
     pattern = re.compile(f"https://(.*)-{arch}-(.*)[0-9]")
 
     @classmethod
@@ -98,6 +98,28 @@ def url_matches(cls, url: str) -> bool:
         return cls.pattern.search(url) is not None
 
 
+class Shellcheck:
+    "Shellcheck's URL pattern"
+    repo = "koalaman/shellcheck"
+
+    os = OS.lower()
+
+    if ARCH in X86_64_ARCH_NAMES or OS == "Darwin":
+        # currently no native arm downloads for darwin
+        # so we just use x86 and rosetta it
+        arch = "x86_64"
+    elif ARCH == "arm64":
+        arch = "aarch64"
+    else:
+        sys.exit(f"Unsupported OS {OS}")
+
+    pattern = f"{os}.{arch}.tar.xz"
+
+    @classmethod
+    def url_matches(cls, url: str) -> bool:
+        return cls.pattern in url
+
+
 class Tool(enum.Enum):
     "Maps the dev tools we want to their repos and respective URL matchers."
 
@@ -109,7 +131,8 @@ def __init__(self, repo: str, matcher: Callable[[str], bool]):
     ct = "helm/chart-testing", StandardTool.url_matches
     conftest = "open-policy-agent/conftest", StandardTool.url_matches
     promtool = "prometheus/prometheus", StandardTool.url_matches
-    shellcheck = "koalaman/shellcheck", StandardTool.url_matches
+
+    shellcheck = Shellcheck.repo, Shellcheck.url_matches
 
     noobaa = Nooba.repo, Nooba.url_matches
 
@@ -199,6 +222,7 @@ def get_latest_assets(repo: str, exact: str = "") -> Iterable[ReleaseAsset]:
     help=f"The executable to retrieve the URL for.\nSupported: {', '.join(CLI_NAMES)}",
     choices=CLI_NAMES,
 )
+parser.add_argument("-v", "--verbose", action="store_true", default=False)
 
 if __name__ == "__main__":
     args = parser.parse_args()
@@ -218,5 +242,9 @@ def get_latest_assets(repo: str, exact: str = "") -> Iterable[ReleaseAsset]:
         if tool.matcher(asset.url):
             print(asset.url)
             sys.exit()
+        elif args.verbose:
+            sys.stderr.write(
+                f"{asset.name} at {asset.url} does not match pattern for {software}\n"
+            )
 
     sys.exit(f"No matching download URL found for {software} on {OS} {ARCH}")

scripts/install_dev_tools.sh

@@ -111,7 +111,7 @@ function should_cli_be_installed(){
 # Function to safely remove temporary files and temporary download dir
 # Argument is optional exit value to propagate it after cleanup
 function cleanup_and_exit() {
-    local exit_val=$1
+    local exit_val=${1:-1}
     if [ -z "${DWN_DIR}" ]; then
         echo "cleanup_and_exit(): Temp download dir not provided !" >&2
     else
@@ -120,14 +120,12 @@ function cleanup_and_exit() {
           PELORUS_TMP_DIR=$(basename "${DWN_DIR}")
           if [[ "${PELORUS_TMP_DIR}" =~ "${TMP_DIR_PREFIX}"* ]]; then
               echo "Cleaning up temporary files"
-              eval rm -f "${DWN_DIR}/*"
-              rmdir "${DWN_DIR}"
+              rm -rf -- "${DWN_DIR}"
           fi
       fi
     fi
-    # Propagate exit value if was provided
-    [ -n "${exit_val}" ] && exit "$exit_val"
-    exit 0
+    trap - EXIT
+    exit "$exit_val"
 }
 
 function print_help() {
@@ -269,8 +267,8 @@ if should_cli_be_installed "noobaa" "${cli_tools_arr[@]}" && \
       echo "$NOOBAA_CLIENT_URL"
       download_file_from_url "${NOOBAA_CLIENT_URL}" "${DWN_DIR}"
       NOOBAA_CLIENT_PATH="${DWN_DIR}"/$(basename "${NOOBAA_CLIENT_URL}")
-      mv "${NOOBAA_CLIENT_PATH}" "${VENV}/bin/noobaa"
-      chmod +x "${VENV}/bin/noobaa"
+      extract_file_to_dir "${NOOBAA_CLIENT_PATH}" "${VENV}/bin/"
+      mv "${VENV}/bin/noobaa-operator" "${VENV}/bin/noobaa"
 fi
 
 if should_cli_be_installed "operator-sdk" "${cli_tools_arr[@]}" && \
@@ -300,3 +298,5 @@ if should_cli_be_installed "poetry" "${cli_tools_arr[@]}" && \
     ! [ -x "$(command -v "${DEFAULT_VENV}/bin/poetry")" ]; then
         curl -sSL https://install.python-poetry.org | POETRY_HOME="$DEFAULT_VENV" python3 -
 fi
+
+cleanup_and_exit 0