Conversation
m-houston
changed the title
| echo " overrides: $overrides" | ||
| echo " overrideProjectName: $overrideProjectName" | ||
| echo " overrideRoleName: $overrideRoleName" | ||
| echo " targetProject: $targetProject" |
There was a problem hiding this comment.
targetProject is echoed and passed into the jq payload, but it is never set from CLI args or defaults, so it will always be empty. Either add argument parsing for it (and document it) or remove these references to avoid confusion.
| # --overrideRoleName <name> | ||
|
|
||
| # | ||
| # All arguments are required except terraformAction, and internalRef. |
There was a problem hiding this comment.
The script claims “All arguments are required except terraformAction, and internalRef.” but there is no validation after parsing, so it can dispatch with empty required inputs and fail later with hard-to-debug API responses. Add explicit checks for required args (infraRepoName, releaseVersion, targetWorkflow, targetEnvironment, targetComponent, targetAccountGroup) with a clear usage message.
|
|
||
| set -euo pipefail | ||
|
|
||
| # Pre-commit git hook to scan for secrets hard-coded in the codebase. This is a |
There was a problem hiding this comment.
Header comments/usage text describe this as a secrets/gitleaks wrapper (scan-secrets.sh) but the script actually searches for TODO. This is misleading for maintainers—update the description and usage to reflect the TODO scanning behavior.
| local todos="$1" | ||
| local exclude_args="$2" | ||
| local todo_count=$(line_count "$todos") | ||
| shift |
There was a problem hiding this comment.
These changes seem a bit over-eager and not related to the review finding.
| --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" \ |
There was a problem hiding this comment.
The comments have been dropped for no reason
| -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}' |
There was a problem hiding this comment.
The
url_templatevalue has a leading space (" https://..."), which will produce an invalid autolink URL. Remove the leading whitespace so generated Jira links are correct.
| @@ -424,6 +437,11 @@ | |||
| function docker-push-container() { | |||
|
|
|||
| if [ "${PUBLISH_CONTAINER_IMAGE:-true}" = "true" ]; then | |||
There was a problem hiding this comment.
docker-push-container uses DOCKER_IMAGE without validating it is set; if it's empty, docker push will fail with a confusing error. Add a guard that errors clearly when DOCKER_IMAGE is missing.
| @@ -397,10 +410,10 @@ | |||
|
|
|||
| # Run the container build script first | |||
| echo "Running build.sh in ${dir}..." | |||
There was a problem hiding this comment.
current_diris assigned withoutlocal, which leaks a global variable into the library scope. Declare itlocal(and ideally use a trap or subshell) so callers don't get unexpected variable collisions.
| return 1 | ||
| fi | ||
|
|
||
| local container_name="${CONTAINER_NAME:-$(basename "$dir")}" |
There was a problem hiding this comment.
docker-calculate-image-name assumes AWS_ACCOUNT_ID, AWS_REGION, and CONTAINER_IMAGE_PREFIX are set (per the comment), but doesn't validate them. Add explicit checks so it fails fast with a clear error instead of emitting a malformed image reference.
| ./build.sh | ||
|
|
||
| # Build the Docker image | ||
| echo "Building container image..." |
There was a problem hiding this comment.
docker-build-container uses -t "${DOCKER_IMAGE}" but never validates that DOCKER_IMAGE is set/non-empty, so callers can end up running an invalid docker buildx command. Add an explicit check for DOCKER_IMAGE early (similar to the BASE_IMAGE check).
1 participant
Description
Copilot identified a number of issues in files synchronised from the repo template. These changes are a backport of fixes for those findings.
Please review carefully to ensure all these changes are sensible.
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.