-
Notifications
You must be signed in to change notification settings - Fork 7
Address production promotion review follow-ups #359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -290,6 +290,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}")" | ||||||
|
|
@@ -448,13 +451,9 @@ 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}" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
(companion change: update the error message on the next if-block to |
||||||
| if [[ -z "${staging_image}" ]]; then | ||||||
| echo "::error::Staging image '${STAGING_IMAGE}' did not contain a usable image reference." | ||||||
| echo "::error::STAGING_IMAGE is not set or is empty." | ||||||
| exit 1 | ||||||
| fi | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | | ||
|
|
@@ -142,7 +154,7 @@ Most apps do not need these: | |
| | Name | Notes | | ||
| | --- | --- | | ||
| | `DOCKER_BUILD_EXTRA_ARGS` | Newline-delimited extra Docker build tokens. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}")" | ||
|
|
@@ -467,13 +470,9 @@ 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}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| if [[ -z "${staging_image}" ]]; then | ||
| echo "::error::Staging image '${STAGING_IMAGE}' did not contain a usable image reference." | ||
| echo "::error::STAGING_IMAGE is not set or is empty." | ||
| exit 1 | ||
| fi | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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_varscalls that follow rather than theenv_check_failed=0line it sits above. Consider moving it to just above the firstcheck_required_varscall so the explanation is co-located with the pattern it documents.