From 7e403d031e226fd7905add79dddcf339b9222842 Mon Sep 17 00:00:00 2001 From: Richard Fussenegger Date: Tue, 21 Feb 2023 11:25:45 +0100 Subject: [PATCH] feat: Add `shellcheck` and `shfmt` Pre-Commit Hooks This adds `shellcheck` and `shfmt` pre-commit hooks. I fixed the most pressing issues that were found, but did not go through the existing scripts otherwise. We are going to need more shell scripts to automate other things, this is why it is a good idea to add these tools to our verification chain. ~This also changes the GHA lint workflow to use the official pre-commit action, which also makes use of caching, and thus should be faster.~ The pre-commit action uses the `--color=always` option, and that option results in the action hanging forever if paired with local hooks, which we have. --- .editorconfig | 21 +++++ .github/workflows/lint.yml | 37 ++++----- .pre-commit-config.yaml | 123 +++++++++++++++------------- container/build_image.sh | 23 ++++-- container/publish_image.sh | 22 ++--- container/start.sh | 59 +++++++------ performance-test/run-locust-test.sh | 18 ++-- requirements-dev.txt | 4 +- 8 files changed, 167 insertions(+), 140 deletions(-) create mode 100644 .editorconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000..c40f560f8 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,21 @@ +root = true + +[*] +charset = utf-8 +end_of_line = lf +indent_size = 4 +indent_style = space +insert_final_newline = true +tab_width = 4 +trim_trailing_whitespace = true + +ij_yaml_indent_sequence_value = false + +[*.{json,md,proto,rst,yaml,yml}] +indent_size = 2 +tab_width = 2 + +# This MUST be a dedicated section, or shfmt is not going to pick it up. +# noinspection EditorConfigOptionRedundancy +[*.sh] +indent_size = 4 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2948bd903..0ef15d1c5 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -2,28 +2,27 @@ name: Lint on: pull_request: - types: [opened, synchronize, reopened] + types: [ opened, synchronize, reopened ] push: - branches: - - main + branches: [ main ] + +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref }} + cancel-in-progress: true jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - - name: Set up Python 3.11 - uses: actions/setup-python@v4 - with: - python-version: 3.11 - - - name: Install dependencies - run: pip install 'pre-commit>=2.2.0' - - # required for pylint - - name: Generate version.py - run: make karapace/version.py - - - name: Run all pre-commit hooks - run: pre-commit run --all-files + - uses: actions/checkout@v3 + - uses: actions/setup-python@v4 + with: + python-version: '3.11' + # required for pylint + - run: make karapace/version.py + - run: pip install pre-commit + - uses: actions/cache@v3 + with: + path: ~/.cache/pre-commit + key: pre-commit-3|${{ env.pythonLocation }}|${{ hashFiles('.pre-commit-config.yaml') }} + - run: pre-commit run --all-files --show-diff-on-failure diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f48a90bdd..6d117bb17 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,61 +1,66 @@ ---- minimum_pre_commit_version: 2.9.2 + repos: - - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.4.0 - hooks: - - id: trailing-whitespace - exclude: ^vendor/|^tests/.*/fixtures/.*|^tests/integration/test_data/.* - - id: end-of-file-fixer - exclude: ^vendor/|^tests/.*/fixtures/.*|^tests/integration/test_data/.* - - id: debug-statements - - - repo: local - hooks: - - id: copyright - name: copyright - language: system - types: [python] - exclude: ^vendor/ - pass_filenames: false - # Lists the Python files that do not have an Aiven copyright. Exits with a - # non-zero exit code if any are found. - entry: bash -c "! git grep --untracked -ELm1 'Copyright \(c\) 20[0-9]{2} Aiven' -- '*.py' ':!*__init__.py'" - - - repo: https://github.com/asottile/pyupgrade - rev: "v3.3.1" - hooks: - - id: pyupgrade - args: ["--py37-plus"] - - - repo: https://github.com/PyCQA/isort - rev: 5.12.0 - hooks: - - id: isort - name: isort (python) - - - repo: https://github.com/psf/black - rev: 22.12.0 - hooks: - - id: black - - - repo: https://github.com/PyCQA/flake8 - rev: 6.0.0 - hooks: - - id: flake8 - - - repo: https://github.com/pre-commit/mirrors-mypy - rev: v0.991 - hooks: - - id: mypy - name: Mypy Karapace - pass_filenames: false - - # https://pre-commit.com/#repository-local-hooks - - repo: https://github.com/PyCQA/pylint - rev: v2.15.10 - hooks: - - id: pylint - exclude: ^vendor/ - args: - - --rcfile=.pylintrc +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.4.0 + hooks: + - id: trailing-whitespace + exclude: ^tests/integration/test_data/.* + - id: end-of-file-fixer + exclude: ^tests/integration/test_data/.* + - id: debug-statements + +# https://pre-commit.com/#repository-local-hooks +- repo: local + hooks: + - id: copyright + name: copyright + language: system + types: [ python ] + pass_filenames: false + # Lists the Python files that do not have an Aiven copyright. Exits with a + # non-zero exit code if any are found. + entry: bash -c "! git grep --untracked -ELm1 'Copyright \(c\) 20[0-9]{2} Aiven' -- '*.py' ':!*__init__.py'" + +- repo: https://github.com/shellcheck-py/shellcheck-py + rev: v0.9.0.2 + hooks: + - id: shellcheck + +- repo: https://github.com/scop/pre-commit-shfmt + rev: v3.6.0-1 + hooks: + - id: shfmt + +- repo: https://github.com/asottile/pyupgrade + rev: v3.3.1 + hooks: + - id: pyupgrade + args: [ --py37-plus ] + +- repo: https://github.com/PyCQA/isort + rev: 5.12.0 + hooks: + - id: isort + +- repo: https://github.com/psf/black + rev: 22.12.0 + hooks: + - id: black + +- repo: https://github.com/PyCQA/flake8 + rev: 6.0.0 + hooks: + - id: flake8 + +- repo: https://github.com/pre-commit/mirrors-mypy + rev: v0.991 + hooks: + - id: mypy + pass_filenames: false + +- repo: https://github.com/PyCQA/pylint + rev: v2.15.10 + hooks: + - id: pylint + args: [ --rcfile=.pylintrc ] diff --git a/container/build_image.sh b/container/build_image.sh index d27dd5104..7c5261950 100755 --- a/container/build_image.sh +++ b/container/build_image.sh @@ -1,4 +1,6 @@ -#!/usr/bin/bash +#!/usr/bin/env bash +set -Eeuo pipefail + # Helper script to generate a karapace image. # # Notes: @@ -23,7 +25,7 @@ COMMIT=$(git rev-parse -q --verify "${ARG_COMMIT}^{commit}") # this is useful if the commit is qualified with the repo name, e.g. aiven/master TAG_NAME=${ARG_TAG_NAME////-} -if [[ -z "${COMMIT}" ]]; then +if [[ -z ${COMMIT} ]]; then echo "Invalid commit provided '${ARG_COMMIT}'" echo "" echo "$0 [:tag]" @@ -31,17 +33,20 @@ if [[ -z "${COMMIT}" ]]; then fi code_checkout=$(mktemp --directory --suffix=-karapace-image) -trap "rm -rf ${code_checkout}" EXIT +trap 'rm -rf "$code_checkout"' EXIT + +git clone "$(dirname "$0")"/.. "${code_checkout}" -git clone $(dirname $0)/.. "${code_checkout}" +pushd "${code_checkout}" +git checkout "$COMMIT" -pushd ${code_checkout} -git checkout $COMMIT +created=$(date --rfc-3339=seconds) +version=$(git describe --always) sudo docker build \ - --build-arg "CREATED=$(date --rfc-3339=seconds)" \ - --build-arg "VERSION=$(git describe --always)" \ + --build-arg "CREATED=$created" \ + --build-arg "VERSION=$version" \ --build-arg "COMMIT=$COMMIT" \ --tag "karapace:${TAG_NAME}" \ --file container/Dockerfile \ - ${code_checkout} + "${code_checkout}" diff --git a/container/publish_image.sh b/container/publish_image.sh index 42fcdc17b..64d56152c 100755 --- a/container/publish_image.sh +++ b/container/publish_image.sh @@ -1,8 +1,8 @@ -#!/usr/bin/bash +#!/usr/bin/env bash +set -Eeuo pipefail # # Helper script to publish an existing image to hub.docker.com # -set -e COLOR_RED=$(tput setaf 1) COLOR_RESET=$(tput setaf reset) @@ -34,16 +34,16 @@ HUB_REPO=karapace DRY_RUN_FLAG=0 if [[ $DRY_RUN_FLAG -eq 1 ]]; then - DRY_RUN=echo + DRY_RUN='echo' else DRY_RUN=sudo fi -object_type=$(git cat-file -t $IMAGE_TAG) -if [[ "${object_type}" == commit ]]; then +object_type=$(git cat-file -t "$IMAGE_TAG") +if [[ ${object_type} == commit ]]; then # make sure we are using the full commit IMAGE_TAG=$(git rev-parse --verify "${IMAGE_TAG}") -elif [[ "${object_type}" == tag ]]; then +elif [[ ${object_type} == tag ]]; then git verify-tag "${IMAGE_TAG}" || die "The git tag '${IMAGE_TAG}' is not signed or the signature could not be validated" else # Only release from commit or tags @@ -59,11 +59,11 @@ HUB_IMAGE="${HUB_USER_REPO}:${IMAGE_TAG}" sudo docker pull ${HUB_USER_REPO} -sudo docker image inspect "${IMAGE_NAME_TAG}" &> /dev/null || die "image '${IMAGE_NAME_TAG}' doesn't exist." +sudo docker image inspect "${IMAGE_NAME_TAG}" &>/dev/null || die "image '${IMAGE_NAME_TAG}' doesn't exist." $DRY_RUN docker tag "${IMAGE_NAME_TAG}" "${HUB_IMAGE}" || die "retagging '${IMAGE_NAME_TAG}' to '${HUB_IMAGE}' failed" FIRST_IMAGE=0 -sudo docker image inspect "${HUB_IMAGE_LATEST}" &> /dev/null || FIRST_IMAGE=1 +sudo docker image inspect "${HUB_IMAGE_LATEST}" &>/dev/null || FIRST_IMAGE=1 if [[ $FIRST_IMAGE -eq 1 ]]; then PUSH_LATEST=1 @@ -76,12 +76,12 @@ else latest_published_commit=$(sudo docker image inspect "${HUB_IMAGE_LATEST}" | jq -r '.[].ContainerConfig.Labels["org.opencontainers.image.version"]') # List the commits that are in $IMAGE_TAG but not in $latest_published_commit - extra_commits_local=$(git rev-list ${latest_published_commit}..${IMAGE_TAG} | wc --lines) + extra_commits_local=$(git rev-list "${latest_published_commit}..${IMAGE_TAG}" | wc --lines) # the opposite - extra_commits_remote=$(git rev-list ${IMAGE_TAG}..${latest_published_commit} | wc --lines) + extra_commits_remote=$(git rev-list "${IMAGE_TAG}..${latest_published_commit}" | wc --lines) if [[ $extra_commits_local -gt 0 && $extra_commits_remote -gt 0 ]]; then - error "The published '${HUB_IMAGE_LATEST}' image and the new image are from diferrent branches." + error "The published '${HUB_IMAGE_LATEST}' image and the new image are from different branches." echo error "'${HUB_IMAGE_LATEST}' is based on '${latest_published_commit}'" error "it has ${extra_commits_remote} additional commits" diff --git a/container/start.sh b/container/start.sh index 15f2b10db..28c97948c 100755 --- a/container/start.sh +++ b/container/start.sh @@ -1,57 +1,56 @@ -#!/bin/bash -set -e +#!/usr/bin/env bash +set -Eeuo pipefail # Configuration is done using environment variables. The environment variable # names are the same as the configuration keys, all letters in caps, and always # start with `KARAPACE_`. - # In the code below the expression ${var+isset} is used to check if the # variable was defined, and ${var-isunset} if not. # # Ref: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02 case $1 in - rest) +rest) # Reexport variables for compatibility - [[ -n "${KARAPACE_REST_ADVERTISED_HOSTNAME+isset}" ]] && export KARAPACE_ADVERTISED_HOSTNAME="${KARAPACE_REST_ADVERTISED_HOSTNAME}" - [[ -n "${KARAPACE_REST_BOOTSTRAP_URI+isset}" ]] && export KARAPACE_BOOTSTRAP_URI="${KARAPACE_REST_BOOTSTRAP_URI}" - [[ -n "${KARAPACE_REST_REGISTRY_HOST+isset}" ]] && export KARAPACE_REGISTRY_HOST="${KARAPACE_REST_REGISTRY_HOST}" - [[ -n "${KARAPACE_REST_REGISTRY_PORT+isset}" ]] && export KARAPACE_REGISTRY_PORT="${KARAPACE_REST_REGISTRY_PORT}" - [[ -n "${KARAPACE_REST_HOST+isset}" ]] && export KARAPACE_HOST="${KARAPACE_REST_HOST}" - [[ -n "${KARAPACE_REST_PORT+isset}" ]] && export KARAPACE_PORT="${KARAPACE_REST_PORT}" - [[ -n "${KARAPACE_REST_ADMIN_METADATA_MAX_AGE+isset}" ]] && export KARAPACE_ADMIN_METADATA_MAX_AGE="${KARAPACE_REST_ADMIN_METADATA_MAX_AGE}" - [[ -n "${KARAPACE_REST_LOG_LEVEL+isset}" ]] && export KARAPACE_LOG_LEVEL="${KARAPACE_REST_LOG_LEVEL}" + [[ -n ${KARAPACE_REST_ADVERTISED_HOSTNAME+isset} ]] && export KARAPACE_ADVERTISED_HOSTNAME="${KARAPACE_REST_ADVERTISED_HOSTNAME}" + [[ -n ${KARAPACE_REST_BOOTSTRAP_URI+isset} ]] && export KARAPACE_BOOTSTRAP_URI="${KARAPACE_REST_BOOTSTRAP_URI}" + [[ -n ${KARAPACE_REST_REGISTRY_HOST+isset} ]] && export KARAPACE_REGISTRY_HOST="${KARAPACE_REST_REGISTRY_HOST}" + [[ -n ${KARAPACE_REST_REGISTRY_PORT+isset} ]] && export KARAPACE_REGISTRY_PORT="${KARAPACE_REST_REGISTRY_PORT}" + [[ -n ${KARAPACE_REST_HOST+isset} ]] && export KARAPACE_HOST="${KARAPACE_REST_HOST}" + [[ -n ${KARAPACE_REST_PORT+isset} ]] && export KARAPACE_PORT="${KARAPACE_REST_PORT}" + [[ -n ${KARAPACE_REST_ADMIN_METADATA_MAX_AGE+isset} ]] && export KARAPACE_ADMIN_METADATA_MAX_AGE="${KARAPACE_REST_ADMIN_METADATA_MAX_AGE}" + [[ -n ${KARAPACE_REST_LOG_LEVEL+isset} ]] && export KARAPACE_LOG_LEVEL="${KARAPACE_REST_LOG_LEVEL}" export KARAPACE_REST=1 - echo "{}" > /opt/karapace/rest.config.json + echo "{}" >/opt/karapace/rest.config.json echo "Starting Karapace REST API" exec python3 -m karapace.karapace_all /opt/karapace/rest.config.json - ;; - registry) + ;; +registry) # Reexport variables for compatibility - [[ -n "${KARAPACE_REGISTRY_ADVERTISED_HOSTNAME+isset}" ]] && export KARAPACE_ADVERTISED_HOSTNAME="${KARAPACE_REGISTRY_ADVERTISED_HOSTNAME}" - [[ -n "${KARAPACE_REGISTRY_BOOTSTRAP_URI+isset}" ]] && export KARAPACE_BOOTSTRAP_URI="${KARAPACE_REGISTRY_BOOTSTRAP_URI}" - [[ -n "${KARAPACE_REGISTRY_HOST+isset}" ]] && export KARAPACE_HOST="${KARAPACE_REGISTRY_HOST}" - [[ -n "${KARAPACE_REGISTRY_PORT+isset}" ]] && export KARAPACE_PORT="${KARAPACE_REGISTRY_PORT}" - [[ -n "${KARAPACE_REGISTRY_CLIENT_ID+isset}" ]] && export KARAPACE_CLIENT_ID="${KARAPACE_REGISTRY_CLIENT_ID}" - [[ -n "${KARAPACE_REGISTRY_GROUP_ID+isset}" ]] && export KARAPACE_GROUP_ID="${KARAPACE_REGISTRY_GROUP_ID}" + [[ -n ${KARAPACE_REGISTRY_ADVERTISED_HOSTNAME+isset} ]] && export KARAPACE_ADVERTISED_HOSTNAME="${KARAPACE_REGISTRY_ADVERTISED_HOSTNAME}" + [[ -n ${KARAPACE_REGISTRY_BOOTSTRAP_URI+isset} ]] && export KARAPACE_BOOTSTRAP_URI="${KARAPACE_REGISTRY_BOOTSTRAP_URI}" + [[ -n ${KARAPACE_REGISTRY_HOST+isset} ]] && export KARAPACE_HOST="${KARAPACE_REGISTRY_HOST}" + [[ -n ${KARAPACE_REGISTRY_PORT+isset} ]] && export KARAPACE_PORT="${KARAPACE_REGISTRY_PORT}" + [[ -n ${KARAPACE_REGISTRY_CLIENT_ID+isset} ]] && export KARAPACE_CLIENT_ID="${KARAPACE_REGISTRY_CLIENT_ID}" + [[ -n ${KARAPACE_REGISTRY_GROUP_ID+isset} ]] && export KARAPACE_GROUP_ID="${KARAPACE_REGISTRY_GROUP_ID}" # Map misspelt environment variable to correct spelling for backwards compatibility. - [[ -n "${KARAPACE_REGISTRY_MASTER_ELIGIBITY+isset}" ]] && export KARAPACE_MASTER_ELIGIBILITY="${KARAPACE_REGISTRY_MASTER_ELIGIBITY}" - [[ -n "${KARAPACE_REGISTRY_MASTER_ELIGIBILITY+isset}" ]] && export KARAPACE_MASTER_ELIGIBILITY="${KARAPACE_REGISTRY_MASTER_ELIGIBILITY}" - [[ -n "${KARAPACE_REGISTRY_TOPIC_NAME+isset}" ]] && export KARAPACE_TOPIC_NAME="${KARAPACE_REGISTRY_TOPIC_NAME}" - [[ -n "${KARAPACE_REGISTRY_COMPATIBILITY+isset}" ]] && export KARAPACE_COMPATIBILITY="${KARAPACE_REGISTRY_COMPATIBILITY}" - [[ -n "${KARAPACE_REGISTRY_LOG_LEVEL+isset}" ]] && export KARAPACE_LOG_LEVEL="${KARAPACE_REGISTRY_LOG_LEVEL}" + [[ -n ${KARAPACE_REGISTRY_MASTER_ELIGIBITY+isset} ]] && export KARAPACE_MASTER_ELIGIBILITY="${KARAPACE_REGISTRY_MASTER_ELIGIBITY}" + [[ -n ${KARAPACE_REGISTRY_MASTER_ELIGIBILITY+isset} ]] && export KARAPACE_MASTER_ELIGIBILITY="${KARAPACE_REGISTRY_MASTER_ELIGIBILITY}" + [[ -n ${KARAPACE_REGISTRY_TOPIC_NAME+isset} ]] && export KARAPACE_TOPIC_NAME="${KARAPACE_REGISTRY_TOPIC_NAME}" + [[ -n ${KARAPACE_REGISTRY_COMPATIBILITY+isset} ]] && export KARAPACE_COMPATIBILITY="${KARAPACE_REGISTRY_COMPATIBILITY}" + [[ -n ${KARAPACE_REGISTRY_LOG_LEVEL+isset} ]] && export KARAPACE_LOG_LEVEL="${KARAPACE_REGISTRY_LOG_LEVEL}" export KARAPACE_REGISTRY=1 - echo "{}" > /opt/karapace/registry.config.json + echo "{}" >/opt/karapace/registry.config.json echo "Starting Karapace Schema Registry" exec python3 -m karapace.karapace_all /opt/karapace/registry.config.json - ;; - *) + ;; +*) echo "usage: start-karapace.sh " exit 0 - ;; + ;; esac wait diff --git a/performance-test/run-locust-test.sh b/performance-test/run-locust-test.sh index 2310faa11..a9e068479 100755 --- a/performance-test/run-locust-test.sh +++ b/performance-test/run-locust-test.sh @@ -1,5 +1,5 @@ -#!/bin/env bash -set -eo pipefail +#!/usr/bin/env bash +set -Eeuo pipefail # The REST proxy address params BASE_URL=${BASE_URL:-http://localhost:8082} @@ -10,14 +10,14 @@ CONCURRENCY=${CONCURRENCY:-50} LOCUST_FILE=${LOCUST_FILE:-"rest-proxy-post-topic-test.py"} GUI_PARAMS="--headless" -if [ ! -z $LOCUST_GUI ]; then +if [[ -n $LOCUST_GUI ]]; then GUI_PARAMS="--autostart" fi TOPIC="${TOPIC}" locust "${GUI_PARAMS}" \ - --run-time "${DURATION}" \ - --users "${CONCURRENCY}" \ - -H "${BASE_URL}" \ - --spawn-rate 0.50 \ - --stop-timeout 5 \ - --locustfile "${LOCUST_FILE}" + --run-time "${DURATION}" \ + --users "${CONCURRENCY}" \ + -H "${BASE_URL}" \ + --spawn-rate 0.50 \ + --stop-timeout 5 \ + --locustfile "${LOCUST_FILE}" diff --git a/requirements-dev.txt b/requirements-dev.txt index 138e94afe..c8ada92fe 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,4 +1,5 @@ # testing +filelock==3.9.0 pytest==6.2.5 pytest-xdist[psutil]==2.2.1 pytest-timeout==1.4.2 @@ -6,9 +7,6 @@ pdbpp==0.10.2 psutil==5.9.0 requests==2.27.1 -# linting -pre-commit>=2.2.0 - # performance test locust==2.13.0