From 577933ea3e744694e3feb9a1dff102ad02974de2 Mon Sep 17 00:00:00 2001 From: Mike Houston Date: Tue, 7 Apr 2026 15:20:14 +0100 Subject: [PATCH 1/3] MAINTENANCE: Fix URL template formatting in auto-link script --- scripts/git-repo/auto-link.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/git-repo/auto-link.sh b/scripts/git-repo/auto-link.sh index a827bd07..bb9ed0fd 100644 --- a/scripts/git-repo/auto-link.sh +++ b/scripts/git-repo/auto-link.sh @@ -6,4 +6,4 @@ curl -L \ -H "Authorization: Bearer $2" \ -H "X-GitHub-Api-Version: 2022-11-28" \ https://api.github.com/repos/NHSDigital/$1/autolinks \ - -d '{"key_prefix":"CCM-","url_template":" https://nhsd-jira.digital.nhs.uk/browse/CCM-","is_alphanumeric":true}' + -d '{"key_prefix":"CCM-","url_template":"https://nhsd-jira.digital.nhs.uk/browse/CCM-","is_alphanumeric":true}' From 20c4c51ad0401481beafc2824b296a019efc3f3f Mon Sep 17 00:00:00 2001 From: Mike Houston Date: Tue, 7 Apr 2026 15:54:30 +0100 Subject: [PATCH 2/3] Apply GitHub Copilot review comments --- .github/actions/trivy-package/action.yaml | 2 +- .../dispatch_internal_repo_workflow.sh | 44 ++++++++++++++++--- .../workflows/pr_create_dynamic_env.disabled | 16 +++---- .../workflows/pr_destroy_dynamic_env.disabled | 14 +++--- scripts/docker/docker.lib.sh | 38 +++++++++++++++- scripts/docker/docker.mk | 10 +++-- scripts/githooks/check-todos.sh | 36 ++++++++------- scripts/terraform/terraform.mk | 10 ++--- src/jekyll-devcontainer/Makefile | 2 +- 9 files changed, 125 insertions(+), 47 deletions(-) diff --git a/.github/actions/trivy-package/action.yaml b/.github/actions/trivy-package/action.yaml index 196dc037..94656569 100644 --- a/.github/actions/trivy-package/action.yaml +++ b/.github/actions/trivy-package/action.yaml @@ -13,6 +13,6 @@ # ./scripts/terraform/trivy-scan.sh --mode package . || exit_code=$? # if [ $exit_code -ne 0 ]; then -# echo "Trivy has detected package vulnerablilites. Please refer to https://nhsd-confluence.digital.nhs.uk/spaces/RIS/pages/1257636917/PLAT-KOP-012+-+Trivy+Pipeline+Vulnerability+Scanning+Exemption" +# echo "Trivy has detected package vulnerabilities. Please refer to https://nhsd-confluence.digital.nhs.uk/spaces/RIS/pages/1257636917/PLAT-KOP-012+-+Trivy+Pipeline+Vulnerability+Scanning+Exemption" # exit 1 # fi diff --git a/.github/scripts/dispatch_internal_repo_workflow.sh b/.github/scripts/dispatch_internal_repo_workflow.sh index a52c1bbe..317c0992 100755 --- a/.github/scripts/dispatch_internal_repo_workflow.sh +++ b/.github/scripts/dispatch_internal_repo_workflow.sh @@ -34,6 +34,35 @@ set -e +usage() { + cat >&2 <<'EOF' +Usage: + ./dispatch_internal_repo_workflow.sh \ + --infraRepoName \ + --releaseVersion \ + --targetWorkflow \ + --targetEnvironment \ + --targetComponent \ + --targetAccountGroup \ + [--terraformAction ] \ + [--internalRef ] \ + [--overrides ] \ + [--overrideProjectName ] \ + [--overrideRoleName ] +EOF +} + +require_arg() { + local name="$1" + local value="$2" + + if [[ -z "$value" ]]; then + echo "[ERROR] Missing required argument: $name" >&2 + usage + exit 1 + fi +} + while [[ $# -gt 0 ]]; do case $1 in --infraRepoName) # Name of the infrastructure repo in NHSDigital org (required) @@ -87,6 +116,13 @@ while [[ $# -gt 0 ]]; do esac done +require_arg "--infraRepoName" "${infraRepoName:-}" +require_arg "--releaseVersion" "${releaseVersion:-}" +require_arg "--targetWorkflow" "${targetWorkflow:-}" +require_arg "--targetEnvironment" "${targetEnvironment:-}" +require_arg "--targetComponent" "${targetComponent:-}" +require_arg "--targetAccountGroup" "${targetAccountGroup:-}" + if [[ -z "$APP_PEM_FILE" ]]; then echo "[ERROR] PEM_FILE environment variable is not set or is empty." exit 1 @@ -166,9 +202,9 @@ echo " internalRef: $internalRef" echo " overrides: $overrides" echo " overrideProjectName: $overrideProjectName" echo " overrideRoleName: $overrideRoleName" -echo " targetProject: $targetProject" DISPATCH_EVENT=$(jq -ncM \ + --arg internalRef "$internalRef" \ --arg infraRepoName "$infraRepoName" \ --arg releaseVersion "$releaseVersion" \ --arg targetEnvironment "$targetEnvironment" \ @@ -179,21 +215,19 @@ DISPATCH_EVENT=$(jq -ncM \ --arg overrides "$overrides" \ --arg overrideProjectName "$overrideProjectName" \ --arg overrideRoleName "$overrideRoleName" \ - --arg targetProject "$targetProject" \ '{ - "ref": "'"$internalRef"'", + "ref": $internalRef, "inputs": ( (if $infraRepoName != "" then { "infraRepoName": $infraRepoName } else {} end) + (if $terraformAction != "" then { "terraformAction": $terraformAction } else {} end) + (if $overrideProjectName != "" then { "overrideProjectName": $overrideProjectName } else {} end) + (if $overrideRoleName != "" then { "overrideRoleName": $overrideRoleName } else {} end) + - (if $targetProject != "" then { "targetProject": $targetProject } else {} end) + { "releaseVersion": $releaseVersion, "targetEnvironment": $targetEnvironment, "targetAccountGroup": $targetAccountGroup, "targetComponent": $targetComponent, - "overrides": $overrides, + "overrides": $overrides } ) }') diff --git a/.github/workflows/pr_create_dynamic_env.disabled b/.github/workflows/pr_create_dynamic_env.disabled index a4de4845..c3de74d7 100644 --- a/.github/workflows/pr_create_dynamic_env.disabled +++ b/.github/workflows/pr_create_dynamic_env.disabled @@ -26,19 +26,19 @@ jobs: --arg infraRepoName "${this_repo_name}" \ --arg releaseVersion "${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \ --arg targetEnvironment "pr${{ github.event.number }}" \ - --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup - --arg targetComponent "component" \ ## Replace with correct targetComponent + --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ + --arg targetComponent "component" \ --arg terraformAction "apply" \ --arg overrides "branch_name=${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \ '{ "ref": "main", "inputs": { "infraRepoName": $infraRepoName, - "releaseVersion", $releaseVersion, - "targetEnvironment", $targetEnvironment, - "targetAccountGroup", $targetAccountGroup, - "targetComponent", $targetComponent, - "terraformAction", $terraformAction, - "overrides", $overrides, + "releaseVersion": $releaseVersion, + "targetEnvironment": $targetEnvironment, + "targetAccountGroup": $targetAccountGroup, + "targetComponent": $targetComponent, + "terraformAction": $terraformAction, + "overrides": $overrides } }') diff --git a/.github/workflows/pr_destroy_dynamic_env.disabled b/.github/workflows/pr_destroy_dynamic_env.disabled index ce7df036..569115ec 100644 --- a/.github/workflows/pr_destroy_dynamic_env.disabled +++ b/.github/workflows/pr_destroy_dynamic_env.disabled @@ -28,17 +28,17 @@ jobs: --arg infraRepoName "${this_repo_name}" \ --arg releaseVersion "main" \ --arg targetEnvironment "pr${{ github.event.number }}" \ - --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup - --arg targetComponent "component" \ ## Replace with correct targetComponent + --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ + --arg targetComponent "component" \ --arg terraformAction "destroy" \ '{ "ref": "main", "inputs": { "infraRepoName": $infraRepoName, - "releaseVersion", $releaseVersion, - "targetEnvironment", $targetEnvironment, - "targetAccountGroup", $targetAccountGroup, - "targetComponent", $targetComponent, - "terraformAction", $terraformAction, + "releaseVersion": $releaseVersion, + "targetEnvironment": $targetEnvironment, + "targetAccountGroup": $targetAccountGroup, + "targetComponent": $targetComponent, + "terraformAction": $terraformAction } }') diff --git a/scripts/docker/docker.lib.sh b/scripts/docker/docker.lib.sh index aa8c8a2c..071978d4 100644 --- a/scripts/docker/docker.lib.sh +++ b/scripts/docker/docker.lib.sh @@ -29,7 +29,7 @@ function docker-build() { _create-effective-dockerfile # The current directory must be changed for the image build script to access # assets that need to be copied - current_dir=$(pwd) + local current_dir=$(pwd) cd "$dir" docker build \ --progress=plain \ @@ -109,6 +109,14 @@ function docker-clean() { local dir=${dir:-$PWD} + if [ -z "${DOCKER_IMAGE:-}" ]; then + echo "DOCKER_IMAGE is not set. Skipping container cleanup." + rm -f \ + .version \ + Dockerfile.effective + return 0 + fi + for version in $(dir="$dir" _get-all-effective-versions) latest; do docker rmi "${DOCKER_IMAGE}:${version}" > /dev/null 2>&1 ||: done @@ -385,6 +393,11 @@ function docker-build-container() { return 1 fi + if [ -z "${DOCKER_IMAGE:-}" ]; then + echo "Error: DOCKER_IMAGE environment variable is required" >&2 + return 1 + fi + if [ ! -f "${dir}/build.sh" ]; then echo "Error: build.sh not found in ${dir}" >&2 return 1 @@ -397,7 +410,7 @@ function docker-build-container() { # Run the container build script first echo "Running build.sh in ${dir}..." - current_dir=$(pwd) + local current_dir=$(pwd) cd "$dir" chmod +x ./build.sh ./build.sh @@ -424,6 +437,11 @@ function docker-build-container() { function docker-push-container() { if [ "${PUBLISH_CONTAINER_IMAGE:-true}" = "true" ]; then + if [ -z "${DOCKER_IMAGE:-}" ]; then + echo "Error: DOCKER_IMAGE environment variable is required" >&2 + return 1 + fi + echo "Pushing to ECR..." echo "Pushing ${DOCKER_IMAGE}..." docker push "${DOCKER_IMAGE}" @@ -440,6 +458,22 @@ function docker-push-container() { # CONTAINER_IMAGE_SUFFIX, ECR_REPO, CONTAINER_NAME, dir (optional) function docker-calculate-image-name() { local dir=${dir:-$PWD} + + if [ -z "${CONTAINER_IMAGE_PREFIX:-}" ]; then + echo "Error: CONTAINER_IMAGE_PREFIX environment variable is required" >&2 + return 1 + fi + + if [ -z "${AWS_ACCOUNT_ID:-}" ]; then + echo "Error: AWS_ACCOUNT_ID environment variable is required" >&2 + return 1 + fi + + if [ -z "${AWS_REGION:-}" ]; then + echo "Error: AWS_REGION environment variable is required" >&2 + return 1 + fi + local container_name="${CONTAINER_NAME:-$(basename "$dir")}" local ecr_repo="${ECR_REPO:-nhs-main-acct-admail}" local image_suffix="${CONTAINER_IMAGE_SUFFIX:-$(docker-get-git-version-suffix)}" diff --git a/scripts/docker/docker.mk b/scripts/docker/docker.mk index d94c95c5..e61657b8 100644 --- a/scripts/docker/docker.mk +++ b/scripts/docker/docker.mk @@ -44,9 +44,13 @@ docker-ghcr-login: # Authenticate Docker with GitHub Container Registry - requir docker-ghcr-login clean:: # Remove container image and resources - required: DOCKER_IMAGE @Development - source scripts/docker/docker.lib.sh; \ - DOCKER_IMAGE="$${DOCKER_IMAGE:-}" \ - docker-clean + @if [ -z "$${DOCKER_IMAGE:-}" ]; then \ + echo "DOCKER_IMAGE is not set. Skipping container cleanup."; \ + else \ + source scripts/docker/docker.lib.sh; \ + DOCKER_IMAGE="$${DOCKER_IMAGE}" \ + docker-clean; \ + fi # ============================================================================== # Quality checks - please DO NOT edit this section! diff --git a/scripts/githooks/check-todos.sh b/scripts/githooks/check-todos.sh index 83b7a80e..430ef0f9 100755 --- a/scripts/githooks/check-todos.sh +++ b/scripts/githooks/check-todos.sh @@ -4,12 +4,12 @@ set -euo pipefail -# Pre-commit git hook to scan for secrets hard-coded in the codebase. This is a -# gitleaks command wrapper. It will run gitleaks natively if it is installed, -# otherwise it will run it in a Docker container. +# Pre-commit git hook to scan for TODO markers in the codebase. +# It checks repository files for TODO entries and fails when a TODO does not +# include a Jira ticket reference. # # Usage: -# $ [options] ./scan-secrets.sh +# $ [options] ./check-todos.sh # # Options: # check=all # check all files in the repository @@ -19,8 +19,8 @@ set -euo pipefail # VERBOSE=true # Show all the executed commands, default is 'false' # # Exit codes: -# 0 - No Todos -# 1 - Todos found or error encountered +# 0 - No TODOs without a Jira ticket reference +# 1 - TODOs without a Jira ticket reference found, or error encountered # 126 - Unknown flag # ============================================================================== @@ -109,7 +109,7 @@ function search_todos() { for ex in "${exclude_args[@]}"; do if [[ "$ex" == --exclude* ]]; then pattern=${ex#--exclude=} - [[ "$file" == $pattern ]] && skip=true && break + [[ "$file" == "$pattern" ]] && skip=true && break fi done @@ -150,8 +150,10 @@ function filter_todos_with_valid_jira_ticket() { function print_output() { local todos="$1" - local exclude_args="$2" - local todo_count=$(line_count "$todos") + shift + local -a exclude_args=("$@") + local todo_count + todo_count=$(line_count "$todos") echo "TODO Check Configuration:" echo "=========================================" @@ -171,7 +173,7 @@ function print_output() { fi if is-arg-true "${VERBOSE:-false}"; then - echo "Grep Exclude Args: $exclude_args" + echo "Grep Exclude Args: ${exclude_args[*]}" fi echo -e "\n=========================================" @@ -184,8 +186,10 @@ function print_output() { echo "No TODOs found." fi - local results=$(filter_todos_with_valid_jira_ticket "$todos") - local results_count=$(line_count "$results") + local results + results=$(filter_todos_with_valid_jira_ticket "$todos") + local results_count + results_count=$(line_count "$results") echo -e "\n=========================================" echo "TODOs without a Jira ticket: $results_count" @@ -204,9 +208,11 @@ function main() { cd "$(git rev-parse --show-toplevel)" local check_mode="${check:-working-tree-changes}" - local exclude_args=$(build_exclude_args) - local todos=$(search_todos "$check_mode" $exclude_args) - print_output "$todos" "$exclude_args" + local -a exclude_args + read -r -a exclude_args <<< "$(build_exclude_args)" + local todos + todos=$(search_todos "$check_mode" "${exclude_args[@]}") + print_output "$todos" "${exclude_args[@]}" } # ============================================================================== diff --git a/scripts/terraform/terraform.mk b/scripts/terraform/terraform.mk index c7dc41cb..7ccee81b 100644 --- a/scripts/terraform/terraform.mk +++ b/scripts/terraform/terraform.mk @@ -14,7 +14,7 @@ terraform-plan: # Plan Terraform changes - mandatory: component=[component_name] project=$(or ${project}, nhs) \ region=$(or ${region}, eu-west-2) \ group=$(or ${group}, dev) \ - opts=$(or ${opts}, ) + opts=$(or ${opts},) terraform-plan-destroy: # Plan Terraform destroy - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, opts @Development # Example: make terraform-plan-destroy component=mycomp environment=myenv group=mygroup @@ -25,7 +25,7 @@ terraform-plan-destroy: # Plan Terraform destroy - mandatory: component=[compone project=$(or ${project}, nhs) \ region=$(or ${region}, eu-west-2) \ group=$(or ${group}, dev) \ - opts=$(or ${opts}, ) + opts=$(or ${opts},) terraform-apply: # Apply Terraform changes - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, build_id, opts @Development # Example: make terraform-apply component=mycomp environment=myenv group=mygroup @@ -36,8 +36,8 @@ terraform-apply: # Apply Terraform changes - mandatory: component=[component_nam project=$(or ${project}, nhs) \ region=$(or ${region}, eu-west-2) \ group=$(or ${group}, dev) \ - build_id=$(or ${build_id}, ) \ - opts=$(or ${opts}, ) + build_id=$(or ${build_id},) \ + opts=$(or ${opts},) terraform-destroy: # Destroy Terraform resources - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, opts @Development # Example: make terraform-destroy component=mycomp environment=myenv group=mygroup @@ -48,7 +48,7 @@ terraform-destroy: # Destroy Terraform resources - mandatory: component=[compone project=$(or ${project}, nhs) \ region=$(or ${region}, eu-west-2) \ group=$(or ${group}, dev) \ - opts=$(or ${opts}, ) + opts=$(or ${opts},) terraform-output: # Get Terraform outputs - mandatory: component=[component_name], environment=[environment]; optional: project, region, group @Development # Example: make terraform-output component=mycomp environment=myenv group=mygroup diff --git a/src/jekyll-devcontainer/Makefile b/src/jekyll-devcontainer/Makefile index e54c9f80..2f4fe75a 100644 --- a/src/jekyll-devcontainer/Makefile +++ b/src/jekyll-devcontainer/Makefile @@ -1,5 +1,5 @@ build: - make -C ../../ Makefile version + $(MAKE) -C ../../ version-create-effective-file dir=. npm install -g @devcontainers/cli ver=$$(head -n 1 ../../.version 2> /dev/null || echo unknown); \ verb=$$(echo $$ver | sed 's/\+.*//'); \ From 24fd323b60fc025f8f7a3a77994d4562481f3296 Mon Sep 17 00:00:00 2001 From: Mike Houston Date: Wed, 8 Apr 2026 09:43:10 +0100 Subject: [PATCH 3/3] Review: restore some comments that were dropped and intended make target --- .github/workflows/pr_create_dynamic_env.disabled | 4 ++-- .github/workflows/pr_destroy_dynamic_env.disabled | 4 ++-- scripts/githooks/check-todos.sh | 2 +- scripts/terraform/terraform.mk | 2 +- src/jekyll-devcontainer/Makefile | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/pr_create_dynamic_env.disabled b/.github/workflows/pr_create_dynamic_env.disabled index c3de74d7..56994168 100644 --- a/.github/workflows/pr_create_dynamic_env.disabled +++ b/.github/workflows/pr_create_dynamic_env.disabled @@ -26,8 +26,8 @@ jobs: --arg infraRepoName "${this_repo_name}" \ --arg releaseVersion "${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \ --arg targetEnvironment "pr${{ github.event.number }}" \ - --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ - --arg targetComponent "component" \ + --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup + --arg targetComponent "component" \ ## Replace with correct targetComponent --arg terraformAction "apply" \ --arg overrides "branch_name=${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \ '{ "ref": "main", diff --git a/.github/workflows/pr_destroy_dynamic_env.disabled b/.github/workflows/pr_destroy_dynamic_env.disabled index 569115ec..9fb4da30 100644 --- a/.github/workflows/pr_destroy_dynamic_env.disabled +++ b/.github/workflows/pr_destroy_dynamic_env.disabled @@ -28,8 +28,8 @@ jobs: --arg infraRepoName "${this_repo_name}" \ --arg releaseVersion "main" \ --arg targetEnvironment "pr${{ github.event.number }}" \ - --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ - --arg targetComponent "component" \ + --arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup + --arg targetComponent "component" \ ## Replace with correct targetComponent --arg terraformAction "destroy" \ '{ "ref": "main", "inputs": { diff --git a/scripts/githooks/check-todos.sh b/scripts/githooks/check-todos.sh index 430ef0f9..90a35f1e 100755 --- a/scripts/githooks/check-todos.sh +++ b/scripts/githooks/check-todos.sh @@ -93,7 +93,7 @@ function build_exclude_args() { function search_todos() { local mode="$1" shift # Shift positional parameters so $@ contains only exclude_args - local exclude_args=("$@") + local -a exclude_args=("$@") local todos="" local files diff --git a/scripts/terraform/terraform.mk b/scripts/terraform/terraform.mk index 7ccee81b..146022b4 100644 --- a/scripts/terraform/terraform.mk +++ b/scripts/terraform/terraform.mk @@ -169,6 +169,6 @@ ${VERBOSE}.SILENT: \ terraform-output \ terraform-plan \ terraform-plan-destroy \ -# terraform-sec \ terraform-validate \ terraform-validate-all \ +# terraform-sec \ diff --git a/src/jekyll-devcontainer/Makefile b/src/jekyll-devcontainer/Makefile index 2f4fe75a..8cae20a1 100644 --- a/src/jekyll-devcontainer/Makefile +++ b/src/jekyll-devcontainer/Makefile @@ -1,5 +1,5 @@ build: - $(MAKE) -C ../../ version-create-effective-file dir=. + $(MAKE) -C ../../ version npm install -g @devcontainers/cli ver=$$(head -n 1 ../../.version 2> /dev/null || echo unknown); \ verb=$$(echo $$ver | sed 's/\+.*//'); \