Merge branch 'bwill/add-templates-shellcheck' into 'master'

Add shellcheck job for vendored CI templates scripts

See merge request gitlab-org/gitlab!80435

.gitlab/ci/ci-templates.gitlab-ci.yml

+templates-shellcheck:
+  extends:
+    - .ci-templates:rules:shellcheck
+    - .default-before_script
+    - .default-retry
+    - .ruby-cache
+    - .use-pg13
+  stage: test
+  needs:
+    - setup-test-env
+  script:
+    - apt update && apt install -y shellcheck=0.7.1-1+deb11u1
+    - bundle exec scripts/lint_templates_bash.rb

.gitlab/ci/rules.gitlab-ci.yml

@@ -147,6 +147,9 @@
   - "scripts/trigger-build.rb"
   - "{,ee/,jh/}{bin,config}/**/*.rb"
 
+.ci-templates-patterns: &ci-templates-patterns
+  - "lib/gitlab/ci/templates/**/*.gitlab-ci.yml"
+
 .ci-qa-patterns: &ci-qa-patterns
   - ".gitlab-ci.yml"
   - ".gitlab/ci/frontend.gitlab-ci.yml"
@@ -441,6 +444,9 @@
   - "GITLAB_WORKHORSE_VERSION"
   - "workhorse/**/*"
   - ".gitlab/ci/workhorse.gitlab-ci.yml"
+  # CI Templates changes
+  - "scripts/lint_templates_bash.rb"
+  - "lib/gitlab/ci/templates/**/*.gitlab-ci.yml"
 
 .danger-patterns: &danger-patterns
   - "Dangerfile"
@@ -609,6 +615,15 @@
       when: manual
       allow_failure: true
 
+######################
+# CI Templates Rules #
+######################
+.ci-templates:rules:shellcheck:
+  rules:
+    - changes: *ci-templates-patterns
+    - changes:
+        - scripts/lint_templates_bash.rb
+
 ##################
 # Delivery rules #
 ##################

doc/development/cicd/templates.md

@@ -105,6 +105,16 @@ Additional points to keep in mind when authoring templates:
 To make templates easier to follow, templates should all use clear syntax styles,
 with a consistent format.
 
+The `before_script`, `script`, and `after_script` keywords of every job are linted
+using [ShellCheck](https://www.shellcheck.net/) and should follow the
+[Shell scripting standards and style guidelines](../shell_scripting_guide/index.md)
+as much as possible.
+
+ShellCheck assumes that the script is designed to run using [Bash](https://www.gnu.org/software/bash/).
+Templates which use scripts for shells that aren't compatible with the Bash ShellCheck
+rules can be excluded from ShellCheck linting. To exclude a script, add it to the
+`EXCLUDED_TEMPLATES` list in [`scripts/lint_templates_bash.rb`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/scripts/lint_templates_bash.rb).
+
 #### Do not hardcode the default branch
 
 Use [`$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH`](../../ci/variables/predefined_variables.md)
@@ -219,6 +229,30 @@ job1:
     - echo ${ERROR_MESSAGE}
 ```
 
+#### Use all-caps naming for non-local variables
+
+If you are expecting a variable to be provided via the CI/CD settings, or via the
+`variables` keyword, that variable must use all-caps naming with underscores (`_`)
+separating words.
+
+```yaml
+.with_login:
+  before_script:
+    # SECRET_TOKEN should be provided via the project settings
+    - docker login -u my-user -p "$SECRET_TOKEN my-registry
+```
+
+Lower-case naming can optionally be used for variables which are defined locally in
+one of the `script` keywords:
+
+```yaml
+job1:
+  script:
+    - response="$(curl "https://example.com/json")"
+    - message="$(echo "$response" | jq -r .message)"
+    - 'echo "Server responded with: $message"'
+```
+
 ### Backward compatibility
 
 A template might be dynamically included with the `include:template:` keyword. If

lib/gitlab/ci/templates/Android-Fastlane.gitlab-ci.yml

@@ -4,8 +4,14 @@
 # https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Android-Fastlane.gitlab-ci.yml
 
 # Read more about how to use this script on this blog post https://about.gitlab.com/2019/01/28/android-publishing-with-gitlab-and-fastlane/
-# You will also need to configure your build.gradle, Dockerfile, and fastlane configuration to make this work.
 # If you are looking for a simpler template that does not publish, see the Android template.
+# You will also need to configure your build.gradle, Dockerfile, and fastlane configuration to make this work.
+
+# The following environment variables also need to be defined via the CI/CD settings:
+#
+# - $signing_jks_file_hex: A hex-encoded Java keystore file containing your signing keys.
+#   To encode this file, use `xxd -p <your-keystore-file>.jks` and save the output as `$signing_jks_file_hex`
+# - $google_play_service_account_api_key_json: Your Google Play service account credentials - https://docs.fastlane.tools/getting-started/android/setup/#collect-your-google-credentials
 
 stages:
   - environment
@@ -41,20 +47,21 @@ ensureContainer:
   before_script:
     - "mkdir -p ~/.docker && echo '{\"experimental\": \"enabled\"}' > ~/.docker/config.json"
     - docker login -u gitlab-ci-token -p $CI_JOB_TOKEN $CI_REGISTRY
-    # Skip update container `script` if the container already exists
-    # via https://gitlab.com/gitlab-org/gitlab-foss/issues/26866#note_97609397 -> https://stackoverflow.com/a/52077071/796832
-    - docker manifest inspect $CI_REGISTRY_IMAGE:$CI_COMMIT_REF_SLUG > /dev/null && exit || true
-
+    - |
+      if docker manifest inspect $CI_REGISTRY_IMAGE:$CI_COMMIT_REF_SLUG > /dev/null; then
+        echo 'Skipping job since there is already an image with this tag'
+        exit 0
+      fi
 
 .build_job:
   image: $CI_REGISTRY_IMAGE:$CI_COMMIT_REF_SLUG
   stage: build
   before_script:
-    # We store this binary file in a variable as hex with this command: `xxd -p android-app.jks`
+    # We store this binary file in a project variable as hex with this command: `xxd -p android-app.jks`
     # Then we convert the hex back to a binary file
     - echo "$signing_jks_file_hex" | xxd -r -p - > android-signing-keystore.jks
-    - "export VERSION_CODE=$CI_PIPELINE_IID && echo $VERSION_CODE"
-    - "export VERSION_SHA=`echo ${CI_COMMIT_SHA:0:8}` && echo $VERSION_SHA"
+    - export VERSION_CODE="$CI_PIPELINE_IID" && echo "$VERSION_CODE"
+    - export VERSION_SHA="${CI_COMMIT_SHA:0:8}" && echo "$VERSION_SHA"
   after_script:
     - rm -f android-signing-keystore.jks || true
   artifacts:

lib/gitlab/ci/templates/Dart.gitlab-ci.yml

@@ -18,7 +18,7 @@ cache:
     - .pub-cache/global_packages
 
 before_script:
-  - export PATH="$PATH":"~/.pub-cache/bin"
+  - export PATH="$PATH:$HOME/.pub-cache/bin"
   - pub get --no-precompile
 
 test:

lib/gitlab/ci/templates/Flutter.gitlab-ci.yml

@@ -8,7 +8,7 @@ code_quality:
   image: "cirrusci/flutter:1.22.5"
   before_script:
     - pub global activate dart_code_metrics
-    - export PATH="$PATH":"$HOME/.pub-cache/bin"
+    - export PATH="$PATH:$HOME/.pub-cache/bin"
   script:
     - metrics lib -r codeclimate  > gl-code-quality-report.json
   artifacts:
@@ -20,7 +20,7 @@ test:
   image: "cirrusci/flutter:1.22.5"
   before_script:
     - pub global activate junitreport
-    - export PATH="$PATH":"$HOME/.pub-cache/bin"
+    - export PATH="$PATH:$HOME/.pub-cache/bin"
   script:
     - flutter test --machine --coverage | tojunit -o report.xml
     - lcov --summary coverage/lcov.info

lib/gitlab/ci/templates/Go.gitlab-ci.yml

@@ -16,9 +16,9 @@ variables:
 # repository in /go/src/gitlab.com/namespace/project
 # Thus, making a symbolic link corrects this.
 before_script:
-  - mkdir -p $GOPATH/src/$(dirname $REPO_NAME)
-  - ln -svf $CI_PROJECT_DIR $GOPATH/src/$REPO_NAME
-  - cd $GOPATH/src/$REPO_NAME
+  - mkdir -p "$GOPATH/src/$(dirname $REPO_NAME)"
+  - ln -svf "$CI_PROJECT_DIR" "$GOPATH/src/$REPO_NAME"
+  - cd "$GOPATH/src/$REPO_NAME"
 
 stages:
   - test

lib/gitlab/ci/templates/Gradle.gitlab-ci.yml

@@ -17,7 +17,8 @@ variables:
   GRADLE_OPTS: "-Dorg.gradle.daemon=false"
 
 before_script:
-  - export GRADLE_USER_HOME=`pwd`/.gradle
+  - GRADLE_USER_HOME="$(pwd)/.gradle"
+  - export GRADLE_USER_HOME
 
 build:
   stage: build

lib/gitlab/ci/templates/Grails.gitlab-ci.yml

@@ -23,8 +23,8 @@ variables:
 before_script:
   - apt-get update -qq && apt-get install -y -qq unzip
   - curl -sSL https://get.sdkman.io | bash
-  - echo sdkman_auto_answer=true > /root/.sdkman/etc/config
-  - source /root/.sdkman/bin/sdkman-init.sh
+  - echo sdkman_auto_answer=true > ~/.sdkman/etc/config
+  - source ~/.sdkman/bin/sdkman-init.sh
   - sdk install gradle $GRADLE_VERSION < /dev/null
   - sdk use gradle $GRADLE_VERSION
   # As it's not a good idea to version gradle.properties feel free to add your
@@ -36,7 +36,7 @@ before_script:
   # Be aware that if you are using Angular profile,
   # Bower cannot be run as root if you don't allow it before.
   # Feel free to remove next line if you are not using Bower
-  - echo {\"allow_root\":true} > /root/.bowerrc
+  - echo '{"allow_root":true}' > ~/.bowerrc
 
 # This build job does the full grails pipeline
 # (compile, test, integrationTest, war, assemble).

lib/gitlab/ci/templates/Jobs/Browser-Performance-Testing.gitlab-ci.yml

@@ -14,15 +14,16 @@ browser_performance:
   script:
     - |
       if ! docker info &>/dev/null; then
-        if [ -z "$DOCKER_HOST" -a "$KUBERNETES_PORT" ]; then
+        if [ -z "$DOCKER_HOST" ] && [ -n "$KUBERNETES_PORT" ]; then
           export DOCKER_HOST='tcp://localhost:2375'
         fi
       fi
-    - export CI_ENVIRONMENT_URL=$(cat environment_url.txt)
+    - CI_ENVIRONMENT_URL="$(cat environment_url.txt)"
+    - export CI_ENVIRONMENT_URL
     - mkdir gitlab-exporter
     # Busybox wget does not support proxied HTTPS, get the real thing.
     # See https://gitlab.com/gitlab-org/gitlab/-/issues/287611.
-    - (env | grep -i _proxy= 2>&1 >/dev/null) && apk --no-cache add wget
+    - (env | grep -i _proxy= >/dev/null 2>&1) && apk --no-cache add wget
     - wget -O gitlab-exporter/index.js https://gitlab.com/gitlab-org/gl-performance/raw/1.1.0/index.js
     - mkdir sitespeed-results
     - |

lib/gitlab/ci/templates/Jobs/Browser-Performance-Testing.latest.gitlab-ci.yml

@@ -14,7 +14,7 @@ browser_performance:
   script:
     - |
       if ! docker info &>/dev/null; then
-        if [ -z "$DOCKER_HOST" -a "$KUBERNETES_PORT" ]; then
+        if [ -z "$DOCKER_HOST" ] && [ -n "$KUBERNETES_PORT" ]; then
           export DOCKER_HOST='tcp://localhost:2375'
         fi
       fi
@@ -22,7 +22,7 @@ browser_performance:
     - mkdir gitlab-exporter
     # Busybox wget does not support proxied HTTPS, get the real thing.
     # See https://gitlab.com/gitlab-org/gitlab/-/issues/287611.
-    - (env | grep -i _proxy= 2>&1 >/dev/null) && apk --no-cache add wget
+    - (env | grep -i _proxy= >/dev/null 2>&1) && apk --no-cache add wget
     - wget -O gitlab-exporter/index.js https://gitlab.com/gitlab-org/gl-performance/raw/1.1.0/index.js
     - mkdir sitespeed-results
     - |

lib/gitlab/ci/templates/Jobs/Code-Quality.gitlab-ci.yml

@@ -13,7 +13,7 @@ code_quality:
     - export SOURCE_CODE=$PWD
     - |
       if ! docker info &>/dev/null; then
-        if [ -z "$DOCKER_HOST" -a "$KUBERNETES_PORT" ]; then
+        if [ -z "$DOCKER_HOST" ] && [ -n "$KUBERNETES_PORT" ]; then
           export DOCKER_HOST='tcp://localhost:2375'
         fi
       fi

lib/gitlab/ci/templates/Jobs/Load-Performance-Testing.gitlab-ci.yml

@@ -14,7 +14,7 @@ load_performance:
   script:
     - |
       if ! docker info &>/dev/null; then
-        if [ -z "$DOCKER_HOST" -a "$KUBERNETES_PORT" ]; then
+        if [ -z "$DOCKER_HOST" ] && [ -n "$KUBERNETES_PORT" ]; then
           export DOCKER_HOST='tcp://localhost:2375'
         fi
       fi

lib/gitlab/ci/templates/Jobs/Secret-Detection.gitlab-ci.yml

@@ -50,11 +50,8 @@ secret_detection:
         git fetch --depth=2 origin $CI_COMMIT_REF_NAME
       else
         # determine commit range so that we can fetch the appropriate depth
-        set +e # ignore exit on failure
-        git log --no-merges --pretty=format:"%H" ${CI_COMMIT_BEFORE_SHA}..${CI_COMMIT_SHA} >${CI_COMMIT_SHA}_commit_list.txt
-
-        # check the exit code of the previous command to determine if we need to limit the commit_list.txt to CI_COMMIT_SHA.
-        if [ $? -ne 0 ];
+        # check the exit code to determine if we need to limit the commit_list.txt to CI_COMMIT_SHA.
+        if ! git log --no-merges --pretty=format:"%H" ${CI_COMMIT_BEFORE_SHA}..${CI_COMMIT_SHA} >${CI_COMMIT_SHA}_commit_list.txt;
         then
           echo "unable to determine commit range, limiting to ${CI_COMMIT_SHA}"
           echo ${CI_COMMIT_SHA} >${CI_COMMIT_SHA}_commit_list.txt
@@ -64,9 +61,6 @@ secret_detection:
           echo >> ${CI_COMMIT_SHA}_commit_list.txt
         fi
 
-        # re-enable exit on failure
-        set -e
-
         # we need to extend the git fetch depth to the number of commits + 1 for the following reasons:
         # to include the parent commit of the base commit in this MR/Push event. This is needed because
         # `git diff -p` needs something to compare changes in that commit against

lib/gitlab/ci/templates/Pages/HTML.gitlab-ci.yml

@@ -8,7 +8,7 @@ pages:
   stage: deploy
   script:
     - mkdir .public
-    - cp -r * .public
+    - cp -r ./* .public
     - rm -rf public
     - mv .public public
   artifacts:

lib/gitlab/ci/templates/Python.gitlab-ci.yml

@@ -47,7 +47,8 @@ run:
 pages:
   script:
     - pip install sphinx sphinx-rtd-theme
-    - cd doc ; make html
+    - cd doc
+    - make html
     - mv build/html/ ../public/
   artifacts:
     paths:

lib/gitlab/ci/templates/Terraform/Base.gitlab-ci.yml

@@ -24,19 +24,19 @@ cache:
 .init: &init
   stage: init
   script:
-    - cd ${TF_ROOT}
+    - cd "${TF_ROOT}"
     - gitlab-terraform init
 
 .validate: &validate
   stage: validate
   script:
-    - cd ${TF_ROOT}
+    - cd "${TF_ROOT}"
     - gitlab-terraform validate
 
 .build: &build
   stage: build
   script:
-    - cd ${TF_ROOT}
+    - cd "${TF_ROOT}"
     - gitlab-terraform plan
     - gitlab-terraform plan-json
   artifacts:
@@ -48,7 +48,7 @@ cache:
 .deploy: &deploy
   stage: deploy
   script:
-    - cd ${TF_ROOT}
+    - cd "${TF_ROOT}"
     - gitlab-terraform apply
   when: manual
   only:
@@ -58,6 +58,6 @@ cache:
 .destroy: &destroy
   stage: cleanup
   script:
-    - cd ${TF_ROOT}
+    - cd "${TF_ROOT}"
     - gitlab-terraform destroy
   when: manual

lib/gitlab/ci/templates/Verify/Accessibility.gitlab-ci.yml

@@ -14,7 +14,8 @@ stages:
 a11y:
   stage: accessibility
   image: registry.gitlab.com/gitlab-org/ci-cd/accessibility:6.1.1
-  script: /gitlab-accessibility.sh $a11y_urls
+  script:
+    - /gitlab-accessibility.sh "$a11y_urls"
   allow_failure: true
   artifacts:
     when: always

lib/gitlab/ci/templates/Verify/Browser-Performance.gitlab-ci.yml

@@ -25,7 +25,7 @@ browser_performance:
     - mkdir gitlab-exporter
     # Busybox wget does not support proxied HTTPS, get the real thing.
     # See https://gitlab.com/gitlab-org/gitlab/-/issues/287611.
-    - (env | grep -i _proxy= 2>&1 >/dev/null) && apk --no-cache add wget
+    - (env | grep -i _proxy= >/dev/null 2>&1) && apk --no-cache add wget
     - wget -O ./gitlab-exporter/index.js https://gitlab.com/gitlab-org/gl-performance/raw/1.1.0/index.js
     - mkdir sitespeed-results
     - |

lib/gitlab/ci/templates/Verify/Browser-Performance.latest.gitlab-ci.yml

@@ -25,7 +25,7 @@ browser_performance:
     - mkdir gitlab-exporter
     # Busybox wget does not support proxied HTTPS, get the real thing.
     # See https://gitlab.com/gitlab-org/gitlab/-/issues/287611.
-    - (env | grep -i _proxy= 2>&1 >/dev/null) && apk --no-cache add wget
+    - (env | grep -i _proxy= >/dev/null 2>&1) && apk --no-cache add wget
     - wget -O ./gitlab-exporter/index.js https://gitlab.com/gitlab-org/gl-performance/raw/1.1.0/index.js
     - mkdir sitespeed-results
     - |

scripts/lint_templates_bash.rb

+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require_relative '../config/environment'
+require 'open3'
+
+module LintTemplatesBash
+  module_function
+
+  EXCLUDED_RULES = [
+    "SC2046", "SC2086", # will be fixed later: https://gitlab.com/gitlab-org/gitlab/-/issues/352973
+    "SC1090", "SC1091", # we do not have access to sourced files for analysis.
+    "SC2154", # Referencing undefined variables is common and adding per-line exceptions for them is unintuitive for end-users
+    "SC2164" # CI/CD automatically fails if attempting to change to a directory which does not exist.
+  ].join(",").freeze
+
+  EXCLUDED_TEMPLATES = [
+    "dotNET.gitlab-ci.yml" # Powershell
+  ].freeze
+
+  def run
+    failed_templates = Gitlab::Template::GitlabCiYmlTemplate.all.filter_map do |template|
+      next if EXCLUDED_TEMPLATES.include?(template.full_name)
+
+      success = check_template(template)
+
+      template.full_name unless success
+    end
+
+    if failed_templates.any?
+      puts "The following templates have shellcheck violations:"
+      puts failed_templates.join("\n")
+      exit 1
+    end
+  end
+
+  def process_content(content)
+    Gitlab::Ci::YamlProcessor.new(content).execute
+  end
+
+  def job_script(job)
+    parts = [:before_script, :script, :after_script].map do |key|
+      job[key]&.join("\n")
+    end.compact
+
+    parts.prepend("#!/bin/bash\n").join("\n")
+  end
+
+  def shellcheck(script_content)
+    combined_streams, status = Open3.capture2e("shellcheck --exclude='#{EXCLUDED_RULES}' -", stdin_data: script_content)
+
+    [combined_streams, status.success?]
+  end
+
+  def check_job(job)
+    shellcheck(job_script(job))
+  end
+
+  def check_template(template)
+    parsed = process_content(template.content)
+    results = parsed.jobs.map do |name, job|
+      out, success = check_job(job)
+
+      unless success
+        puts "The '#{name}' job in #{template.full_name} has shellcheck failures:"
+        puts out
+      end
+
+      success
+    end
+
+    results.all?
+  end
+end
+
+LintTemplatesBash.run