Fix downstream copilot review findings (#101)

m-houston · web-flow · commit 273f937ec75e · 2026-04-08T10:21:26.000+01:00
* MAINTENANCE: Fix URL template formatting in auto-link script

* Apply GitHub Copilot review comments

* Review: restore some comments that were dropped and intended make target
diff --git 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
@@ -34,6 +34,35 @@
 
 set -e
 
+usage() {
+  cat >&2 <<'EOF'
+Usage:
+  ./dispatch_internal_repo_workflow.sh \
+    --infraRepoName <repo> \
+    --releaseVersion <version> \
+    --targetWorkflow <workflow.yaml> \
+    --targetEnvironment <env> \
+    --targetComponent <component> \
+    --targetAccountGroup <group> \
+    [--terraformAction <action>] \
+    [--internalRef <ref>] \
+    [--overrides <overrides>] \
+    [--overrideProjectName <name>] \
+    [--overrideRoleName <name>]
+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
@@ -33,12 +33,12 @@ jobs:
             '{ "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
@@ -34,11 +34,11 @@ jobs:
             '{ "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
@@ -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
@@ -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/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-<num>","is_alphanumeric":true}'
+  -d '{"key_prefix":"CCM-","url_template":"https://nhsd-jira.digital.nhs.uk/browse/CCM-<num>","is_alphanumeric":true}'
diff --git 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
 
 # ==============================================================================
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,5 +1,5 @@
 build:
-	make -C ../../ Makefile version
+	$(MAKE) -C ../../ version
 	npm install -g @devcontainers/cli
 	ver=$$(head -n 1 ../../.version 2> /dev/null || echo unknown); \
 	verb=$$(echo $$ver | sed 's/\+.*//'); \
