Skip to content

Commit bfcf55d

Browse files
m-houstonm-houston
committed
Apply GitHub Copilot review comments
1 parent 99f8bec commit bfcf55d

8 files changed

Lines changed: 118 additions & 40 deletions

File tree

‎.github/scripts/dispatch_internal_repo_workflow.sh‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,35 @@
3434

3535
set -e
3636

37+
usage() {
38+
cat >&2 <<'EOF'
39+
Usage:
40+
./dispatch_internal_repo_workflow.sh \
41+
--infraRepoName <repo> \
42+
--releaseVersion <version> \
43+
--targetWorkflow <workflow.yaml> \
44+
--targetEnvironment <env> \
45+
--targetComponent <component> \
46+
--targetAccountGroup <group> \
47+
[--terraformAction <action>] \
48+
[--internalRef <ref>] \
49+
[--overrides <overrides>] \
50+
[--overrideProjectName <name>] \
51+
[--overrideRoleName <name>]
52+
EOF
53+
}
54+
55+
require_arg() {
56+
local name="$1"
57+
local value="$2"
58+
59+
if [[ -z "$value" ]]; then
60+
echo "[ERROR] Missing required argument: $name" >&2
61+
usage
62+
exit 1
63+
fi
64+
}
65+
3766
while [[ $# -gt 0 ]]; do
3867
case $1 in
3968
--infraRepoName) # Name of the infrastructure repo in NHSDigital org (required)
@@ -87,6 +116,13 @@ while [[ $# -gt 0 ]]; do
87116
esac
88117
done
89118

119+
require_arg "--infraRepoName" "${infraRepoName:-}"
120+
require_arg "--releaseVersion" "${releaseVersion:-}"
121+
require_arg "--targetWorkflow" "${targetWorkflow:-}"
122+
require_arg "--targetEnvironment" "${targetEnvironment:-}"
123+
require_arg "--targetComponent" "${targetComponent:-}"
124+
require_arg "--targetAccountGroup" "${targetAccountGroup:-}"
125+
90126
if [[ -z "$APP_PEM_FILE" ]]; then
91127
echo "[ERROR] PEM_FILE environment variable is not set or is empty."
92128
exit 1
@@ -166,9 +202,9 @@ echo " internalRef: $internalRef"
166202
echo " overrides: $overrides"
167203
echo " overrideProjectName: $overrideProjectName"
168204
echo " overrideRoleName: $overrideRoleName"
169-
echo " targetProject: $targetProject"
170205

171206
DISPATCH_EVENT=$(jq -ncM \
207+
--arg internalRef "$internalRef" \
172208
--arg infraRepoName "$infraRepoName" \
173209
--arg releaseVersion "$releaseVersion" \
174210
--arg targetEnvironment "$targetEnvironment" \
@@ -179,21 +215,19 @@ DISPATCH_EVENT=$(jq -ncM \
179215
--arg overrides "$overrides" \
180216
--arg overrideProjectName "$overrideProjectName" \
181217
--arg overrideRoleName "$overrideRoleName" \
182-
--arg targetProject "$targetProject" \
183218
'{
184-
"ref": "'"$internalRef"'",
219+
"ref": $internalRef,
185220
"inputs": (
186221
(if $infraRepoName != "" then { "infraRepoName": $infraRepoName } else {} end) +
187222
(if $terraformAction != "" then { "terraformAction": $terraformAction } else {} end) +
188223
(if $overrideProjectName != "" then { "overrideProjectName": $overrideProjectName } else {} end) +
189224
(if $overrideRoleName != "" then { "overrideRoleName": $overrideRoleName } else {} end) +
190-
(if $targetProject != "" then { "targetProject": $targetProject } else {} end) +
191225
{
192226
"releaseVersion": $releaseVersion,
193227
"targetEnvironment": $targetEnvironment,
194228
"targetAccountGroup": $targetAccountGroup,
195229
"targetComponent": $targetComponent,
196-
"overrides": $overrides,
230+
"overrides": $overrides
197231
}
198232
)
199233
}')

‎.github/workflows/pr_create_dynamic_env.disabled‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ jobs:
2626
--arg infraRepoName "${this_repo_name}" \
2727
--arg releaseVersion "${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \
2828
--arg targetEnvironment "pr${{ github.event.number }}" \
29-
--arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup
30-
--arg targetComponent "component" \ ## Replace with correct targetComponent
29+
--arg targetAccountGroup "nhs-notify-bounded-context-dev" \
30+
--arg targetComponent "component" \
3131
--arg terraformAction "apply" \
3232
--arg overrides "branch_name=${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \
3333
'{ "ref": "main",

‎.github/workflows/pr_destroy_dynamic_env.disabled‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,17 @@ jobs:
2828
--arg infraRepoName "${this_repo_name}" \
2929
--arg releaseVersion "main" \
3030
--arg targetEnvironment "pr${{ github.event.number }}" \
31-
--arg targetAccountGroup "nhs-notify-bounded-context-dev" \ ## Replace with correct targetAccountGroup
32-
--arg targetComponent "component" \ ## Replace with correct targetComponent
31+
--arg targetAccountGroup "nhs-notify-bounded-context-dev" \
32+
--arg targetComponent "component" \
3333
--arg terraformAction "destroy" \
3434
'{ "ref": "main",
3535
"inputs": {
3636
"infraRepoName": $infraRepoName,
37-
"releaseVersion", $releaseVersion,
38-
"targetEnvironment", $targetEnvironment,
39-
"targetAccountGroup", $targetAccountGroup,
40-
"targetComponent", $targetComponent,
41-
"terraformAction", $terraformAction,
37+
"releaseVersion": $releaseVersion,
38+
"targetEnvironment": $targetEnvironment,
39+
"targetAccountGroup": $targetAccountGroup,
40+
"targetComponent": $targetComponent,
41+
"terraformAction": $terraformAction
4242
}
4343
}')
4444

‎scripts/docker/docker.lib.sh‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ function docker-build() {
2929
_create-effective-dockerfile
3030
# The current directory must be changed for the image build script to access
3131
# assets that need to be copied
32-
current_dir=$(pwd)
32+
local current_dir=$(pwd)
3333
cd "$dir"
3434
docker build \
3535
--progress=plain \
@@ -109,6 +109,14 @@ function docker-clean() {
109109

110110
local dir=${dir:-$PWD}
111111

112+
if [ -z "${DOCKER_IMAGE:-}" ]; then
113+
echo "DOCKER_IMAGE is not set. Skipping container cleanup."
114+
rm -f \
115+
.version \
116+
Dockerfile.effective
117+
return 0
118+
fi
119+
112120
for version in $(dir="$dir" _get-all-effective-versions) latest; do
113121
docker rmi "${DOCKER_IMAGE}:${version}" > /dev/null 2>&1 ||:
114122
done
@@ -385,6 +393,11 @@ function docker-build-container() {
385393
return 1
386394
fi
387395

396+
if [ -z "${DOCKER_IMAGE:-}" ]; then
397+
echo "Error: DOCKER_IMAGE environment variable is required" >&2
398+
return 1
399+
fi
400+
388401
if [ ! -f "${dir}/build.sh" ]; then
389402
echo "Error: build.sh not found in ${dir}" >&2
390403
return 1
@@ -397,7 +410,7 @@ function docker-build-container() {
397410

398411
# Run the container build script first
399412
echo "Running build.sh in ${dir}..."
400-
current_dir=$(pwd)
413+
local current_dir=$(pwd)
401414
cd "$dir"
402415
chmod +x ./build.sh
403416
./build.sh
@@ -424,6 +437,11 @@ function docker-build-container() {
424437
function docker-push-container() {
425438

426439
if [ "${PUBLISH_CONTAINER_IMAGE:-true}" = "true" ]; then
440+
if [ -z "${DOCKER_IMAGE:-}" ]; then
441+
echo "Error: DOCKER_IMAGE environment variable is required" >&2
442+
return 1
443+
fi
444+
427445
echo "Pushing to ECR..."
428446
echo "Pushing ${DOCKER_IMAGE}..."
429447
docker push "${DOCKER_IMAGE}"
@@ -440,6 +458,22 @@ function docker-push-container() {
440458
# CONTAINER_IMAGE_SUFFIX, ECR_REPO, CONTAINER_NAME, dir (optional)
441459
function docker-calculate-image-name() {
442460
local dir=${dir:-$PWD}
461+
462+
if [ -z "${CONTAINER_IMAGE_PREFIX:-}" ]; then
463+
echo "Error: CONTAINER_IMAGE_PREFIX environment variable is required" >&2
464+
return 1
465+
fi
466+
467+
if [ -z "${AWS_ACCOUNT_ID:-}" ]; then
468+
echo "Error: AWS_ACCOUNT_ID environment variable is required" >&2
469+
return 1
470+
fi
471+
472+
if [ -z "${AWS_REGION:-}" ]; then
473+
echo "Error: AWS_REGION environment variable is required" >&2
474+
return 1
475+
fi
476+
443477
local container_name="${CONTAINER_NAME:-$(basename "$dir")}"
444478
local ecr_repo="${ECR_REPO:-nhs-main-acct-admail}"
445479
local image_suffix="${CONTAINER_IMAGE_SUFFIX:-$(docker-get-git-version-suffix)}"

‎scripts/docker/docker.mk‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,13 @@ docker-ghcr-login: # Authenticate Docker with GitHub Container Registry - requir
4444
docker-ghcr-login
4545

4646
clean:: # Remove container image and resources - required: DOCKER_IMAGE @Development
47-
source scripts/docker/docker.lib.sh; \
48-
DOCKER_IMAGE="$${DOCKER_IMAGE:-}" \
49-
docker-clean
47+
@if [ -z "$${DOCKER_IMAGE:-}" ]; then \
48+
echo "DOCKER_IMAGE is not set. Skipping container cleanup."; \
49+
else \
50+
source scripts/docker/docker.lib.sh; \
51+
DOCKER_IMAGE="$${DOCKER_IMAGE}" \
52+
docker-clean; \
53+
fi
5054

5155
# ==============================================================================
5256
# Quality checks - please DO NOT edit this section!

‎scripts/githooks/check-todos.sh‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@
44

55
set -euo pipefail
66

7-
# Pre-commit git hook to scan for secrets hard-coded in the codebase. This is a
8-
# gitleaks command wrapper. It will run gitleaks natively if it is installed,
9-
# otherwise it will run it in a Docker container.
7+
# Pre-commit git hook to scan for TODO markers in the codebase.
8+
# It checks repository files for TODO entries and fails when a TODO does not
9+
# include a Jira ticket reference.
1010
#
1111
# Usage:
12-
# $ [options] ./scan-secrets.sh
12+
# $ [options] ./check-todos.sh
1313
#
1414
# Options:
1515
# check=all # check all files in the repository
@@ -19,8 +19,8 @@ set -euo pipefail
1919
# VERBOSE=true # Show all the executed commands, default is 'false'
2020
#
2121
# Exit codes:
22-
# 0 - No Todos
23-
# 1 - Todos found or error encountered
22+
# 0 - No TODOs without a Jira ticket reference
23+
# 1 - TODOs without a Jira ticket reference found, or error encountered
2424
# 126 - Unknown flag
2525

2626
# ==============================================================================
@@ -109,7 +109,7 @@ function search_todos() {
109109
for ex in "${exclude_args[@]}"; do
110110
if [[ "$ex" == --exclude* ]]; then
111111
pattern=${ex#--exclude=}
112-
[[ "$file" == $pattern ]] && skip=true && break
112+
[[ "$file" == "$pattern" ]] && skip=true && break
113113
fi
114114
done
115115

@@ -150,8 +150,10 @@ function filter_todos_with_valid_jira_ticket() {
150150

151151
function print_output() {
152152
local todos="$1"
153-
local exclude_args="$2"
154-
local todo_count=$(line_count "$todos")
153+
shift
154+
local -a exclude_args=("$@")
155+
local todo_count
156+
todo_count=$(line_count "$todos")
155157

156158
echo "TODO Check Configuration:"
157159
echo "========================================="
@@ -171,7 +173,7 @@ function print_output() {
171173
fi
172174

173175
if is-arg-true "${VERBOSE:-false}"; then
174-
echo "Grep Exclude Args: $exclude_args"
176+
echo "Grep Exclude Args: ${exclude_args[*]}"
175177
fi
176178

177179
echo -e "\n========================================="
@@ -184,8 +186,10 @@ function print_output() {
184186
echo "No TODOs found."
185187
fi
186188

187-
local results=$(filter_todos_with_valid_jira_ticket "$todos")
188-
local results_count=$(line_count "$results")
189+
local results
190+
results=$(filter_todos_with_valid_jira_ticket "$todos")
191+
local results_count
192+
results_count=$(line_count "$results")
189193

190194
echo -e "\n========================================="
191195
echo "TODOs without a Jira ticket: $results_count"
@@ -204,9 +208,11 @@ function main() {
204208
cd "$(git rev-parse --show-toplevel)"
205209

206210
local check_mode="${check:-working-tree-changes}"
207-
local exclude_args=$(build_exclude_args)
208-
local todos=$(search_todos "$check_mode" $exclude_args)
209-
print_output "$todos" "$exclude_args"
211+
local -a exclude_args
212+
read -r -a exclude_args <<< "$(build_exclude_args)"
213+
local todos
214+
todos=$(search_todos "$check_mode" "${exclude_args[@]}")
215+
print_output "$todos" "${exclude_args[@]}"
210216
}
211217

212218
# ==============================================================================

‎scripts/terraform/terraform.mk‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ terraform-plan: # Plan Terraform changes - mandatory: component=[component_name]
1414
project=$(or ${project}, nhs) \
1515
region=$(or ${region}, eu-west-2) \
1616
group=$(or ${group}, dev) \
17-
opts=$(or ${opts}, )
17+
opts=$(or ${opts},)
1818

1919
terraform-plan-destroy: # Plan Terraform destroy - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, opts @Development
2020
# Example: make terraform-plan-destroy component=mycomp environment=myenv group=mygroup
@@ -25,7 +25,7 @@ terraform-plan-destroy: # Plan Terraform destroy - mandatory: component=[compone
2525
project=$(or ${project}, nhs) \
2626
region=$(or ${region}, eu-west-2) \
2727
group=$(or ${group}, dev) \
28-
opts=$(or ${opts}, )
28+
opts=$(or ${opts},)
2929

3030
terraform-apply: # Apply Terraform changes - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, build_id, opts @Development
3131
# Example: make terraform-apply component=mycomp environment=myenv group=mygroup
@@ -36,8 +36,8 @@ terraform-apply: # Apply Terraform changes - mandatory: component=[component_nam
3636
project=$(or ${project}, nhs) \
3737
region=$(or ${region}, eu-west-2) \
3838
group=$(or ${group}, dev) \
39-
build_id=$(or ${build_id}, ) \
40-
opts=$(or ${opts}, )
39+
build_id=$(or ${build_id},) \
40+
opts=$(or ${opts},)
4141

4242
terraform-destroy: # Destroy Terraform resources - mandatory: component=[component_name], environment=[environment]; optional: project, region, group, opts @Development
4343
# Example: make terraform-destroy component=mycomp environment=myenv group=mygroup
@@ -48,7 +48,7 @@ terraform-destroy: # Destroy Terraform resources - mandatory: component=[compone
4848
project=$(or ${project}, nhs) \
4949
region=$(or ${region}, eu-west-2) \
5050
group=$(or ${group}, dev) \
51-
opts=$(or ${opts}, )
51+
opts=$(or ${opts},)
5252

5353
terraform-output: # Get Terraform outputs - mandatory: component=[component_name], environment=[environment]; optional: project, region, group @Development
5454
# Example: make terraform-output component=mycomp environment=myenv group=mygroup

‎src/jekyll-devcontainer/Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
build:
2-
make -C ../../ Makefile version
2+
$(MAKE) -C ../../ version-create-effective-file dir=.
33
npm install -g @devcontainers/cli
44
ver=$$(head -n 1 ../../.version 2> /dev/null || echo unknown); \
55
verb=$$(echo $$ver | sed 's/\+.*//'); \

0 commit comments

Comments
 (0)