Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions .github/workflows/cpflow-promote-staging-to-production.yml
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ jobs:
fi
}

# check_required_vars intentionally mutates env_check_failed in this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment accurately explains the subshell-scoping constraint, but it describes the behaviour of the check_required_vars calls that follow rather than the env_check_failed=0 line it sits above. Consider moving it to just above the first check_required_vars call so the explanation is co-located with the pattern it documents.

# shell; keep calls outside subshells so failures aggregate before the
# final exit.
env_check_failed=0

staging_vars="$(list_gvc_env_names "${CPLN_TOKEN_STAGING}" "${CPLN_ORG_STAGING}" "${STAGING_APP_NAME}")"
Expand Down Expand Up @@ -448,11 +451,7 @@ jobs:
copy_image_attempts=$((copy_image_retries + 1))
copy_image_retry_interval=$((10#${COPY_IMAGE_RETRY_INTERVAL}))

if [[ "${STAGING_IMAGE}" == *@* ]]; then
staging_image="${STAGING_IMAGE}"
else
staging_image="${STAGING_IMAGE%%@*}"
fi
staging_image="${STAGING_IMAGE}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the if/else removed, the empty-string check two lines below now fires only when STAGING_IMAGE itself is empty. The error message "did not contain a usable image reference" was written for when the %%@* strip could produce an empty string, so it reads oddly now. A follow-up like the suggestion below would be clearer, but it is not blocking:

Suggested change
staging_image="${STAGING_IMAGE}"
staging_image="${STAGING_IMAGE}"

(companion change: update the error message on the next if-block to "STAGING_IMAGE is not set or is empty; cannot proceed.")

if [[ -z "${staging_image}" ]]; then
echo "::error::Staging image '${STAGING_IMAGE}' did not contain a usable image reference."
exit 1
Expand Down
14 changes: 13 additions & 1 deletion lib/github_flow_templates/.github/cpflow-help.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,23 @@ For the normal generated review-app path, GitHub needs one repository secret:
| --- | --- | --- |
| `CPLN_TOKEN_STAGING` | Repository secret | Control Plane service-account token for the staging/review org. |

For public repositories, use a staging/review token that cannot access
production Control Plane resources. Generated review-app deploys skip fork PR
heads because Docker builds use repository secrets. If a forked change needs a
review app, first move the reviewed change to a trusted branch in this
repository.

No repository variables are required for the standard review-app path when
`.controlplane/controlplane.yml` has exactly one review app entry with
`match_if_app_name_starts_with: true`. cpflow infers the review-app prefix and
staging org from that config.

Review apps run pull request code. Any value mounted through
`cpln://secret/...` can be read by that code after the workload starts, so keep
review-app secret dictionaries limited to disposable databases, review-only
renderer credentials, and license values that are acceptable for review-app
exposure.

Optional overrides exist for forks, clones, and unusual apps:

| Name | Notes |
Expand Down Expand Up @@ -142,7 +154,7 @@ Most apps do not need these:
| Name | Notes |
| --- | --- |
| `DOCKER_BUILD_EXTRA_ARGS` | Newline-delimited extra Docker build tokens. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement. One optional follow-up: also recommend scoping the deploy key to the specific repository being fetched (not just read-only across all repos) to enforce least-privilege more precisely — e.g. "Read-only deploy key scoped to the dependency repository; do not use a personal or org-wide SSH key."

| `DOCKER_BUILD_SSH_KEY` | Private SSH key for Docker builds that fetch private dependencies. |
| `DOCKER_BUILD_SSH_KEY` | Read-only, revocable deploy key for Docker builds that fetch private dependencies. Do not use a personal SSH key. |
| `DOCKER_BUILD_SSH_KNOWN_HOSTS` | SSH known_hosts entries when SSH build hosts are not GitHub.com. |
| `REVIEW_APP_DEPLOYING_ICON_URL` | Cosmetic custom image URL for the animated deploying icon. Set to `none` to use the text fallback icon. |
| `STAGING_APP_BRANCH` | Custom staging branch. The branch must also appear in `cpflow-deploy-staging.yml`'s push filter. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,9 @@ jobs:
fi
}

# check_required_vars intentionally mutates env_check_failed in this
# shell; keep calls outside subshells so failures aggregate before the
# final exit.
env_check_failed=0

staging_vars="$(list_gvc_env_names "${CPLN_TOKEN_STAGING}" "${CPLN_ORG_STAGING}" "${STAGING_APP_NAME}")"
Expand Down Expand Up @@ -467,11 +470,7 @@ jobs:
copy_image_attempts=$((copy_image_retries + 1))
copy_image_retry_interval=$((10#${COPY_IMAGE_RETRY_INTERVAL}))

if [[ "${STAGING_IMAGE}" == *@* ]]; then
staging_image="${STAGING_IMAGE}"
else
staging_image="${STAGING_IMAGE%%@*}"
fi
staging_image="${STAGING_IMAGE}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit as the corresponding change in the non-template workflow: with the if/else removed, the empty-string check below now fires only when STAGING_IMAGE itself is unset. The error message "did not contain a usable image reference" was written for the strip-produced-empty-string case and reads oddly now. Worth updating to something like "STAGING_IMAGE is not set or is empty; cannot proceed." in a follow-up. Not blocking.

if [[ -z "${staging_image}" ]]; then
echo "::error::Staging image '${STAGING_IMAGE}' did not contain a usable image reference."
exit 1
Expand Down
14 changes: 11 additions & 3 deletions spec/command/generate_github_actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,12 @@ def shared_workflow_path(name)
expect(help_md).to include("Before the first staging deploy")
expect(help_md).to include("cpflow setup-app -a")
expect(help_md).to include("app secret policy")
expect(help_md).to include("For public repositories, use a staging/review token")
expect(help_md).to include("production Control Plane resources")
expect(help_md).to include("Generated review-app deploys skip fork PR")
expect(help_md).to include("heads because Docker builds use repository secrets")
expect(help_md).to include("Review apps run pull request code")
expect(help_md).to include("review-app secret dictionaries limited to disposable databases")
expect(help_md).to include("Add it as a secret on the 'production' GitHub Environment")
expect(help_md).to include("permission to manage repository environments and secrets")
expect(help_md).to include("gh secret set CPLN_TOKEN_PRODUCTION --repo OWNER/REPO --env production")
Expand All @@ -703,6 +709,8 @@ def shared_workflow_path(name)
help_md_path = playground.join(".github/cpflow-help.md")
contents = help_md_path.read
expect(contents).to include("DOCKER_BUILD_EXTRA_ARGS")
expect(contents).to include("Read-only, revocable deploy key")
expect(contents).to include("Do not use a personal SSH key")
expect(contents).to include("DOCKER_BUILD_SSH_KNOWN_HOSTS")
end

Expand Down Expand Up @@ -794,11 +802,13 @@ def shared_workflow_path(name)
expect(contents).to include("Production-only variables")
expect(contents).to include("WORKLOAD_NAMES: ${{ steps.workloads.outputs.names }}")
expect(contents).to include("list_workload_env_names()")
expect(contents).to include("check_required_vars intentionally mutates env_check_failed")
expect(contents).to include(
"Production workload '${workload_name}' is missing environment variables that exist in staging"
)
expect(wrapper).to include("WORKLOAD_NAMES: ${{ steps.workloads.outputs.names }}")
expect(wrapper).to include("list_workload_env_names()")
expect(wrapper).to include("check_required_vars intentionally mutates env_check_failed")
expect(wrapper).to include(
"Production workload '${workload_name}' is missing environment variables that exist in staging"
)
Expand Down Expand Up @@ -859,10 +869,8 @@ def shared_workflow_path(name)
"uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5"
)
expect(contents).to include("id: copy-image")
expect(contents).to include('if [[ "${STAGING_IMAGE}" == *@* ]]; then')
expect(contents).to include('staging_image="${STAGING_IMAGE}"')
expect(contents).to include("else")
expect(contents).to include('staging_image="${STAGING_IMAGE%%@*}"')
expect(contents).not_to include('staging_image="${STAGING_IMAGE%%@*}"')
expect(contents).to include('CPLN_TOKEN="${CPLN_TOKEN_STAGING}" cpln image get "${staging_image}"')
expect(contents).to include('if [[ "${staging_image}" == *@* ]]; then')
expect(contents).to include('staging_tag="${staging_image##*@}"')
Expand Down
Loading