Sanitize Control Plane org names in production promotion#759
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe workflow adds a ChangesControl Plane org name normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code ReviewOverall: Solid, well-targeted fix for a confirmed production bug. The approach — normalize early, validate, then propagate sanitized values — is the right one. All 9 downstream consumer references have been updated consistently with no missed usages. What this does
The two remaining
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7975fee3e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value="${value//$'\r'/}" | ||
| value="${value//$'\n'/}" |
There was a problem hiding this comment.
If a repository variable contains a CR/LF anywhere other than the accidental trailing line ending this change is trying to tolerate, these substitutions concatenate the pieces before validation, so foo\nbar becomes foobar and passes the regex. Because every subsequent deploy, copy, health, and rollback step uses these normalized outputs, a malformed variable can target a different valid Control Plane org/registry instead of failing early; strip only boundary line endings or fail when CR/LF appears inside the name.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| sanitize_control_plane_name() { | ||
| local value="$1" | ||
| value="${value//$'\r'/}" | ||
| value="${value//$'\n'/}" | ||
| printf '%s' "${value}" | ||
| } |
There was a problem hiding this comment.
The function strips \r and \n but not spaces or tabs. A value like " my-org " (copy-pasted from a UI with surrounding spaces) passes through sanitized but then fails the regex with a message that says "remove whitespace" — which is technically correct but unhelpful.
Consider also stripping leading/trailing whitespace:
| sanitize_control_plane_name() { | |
| local value="$1" | |
| value="${value//$'\r'/}" | |
| value="${value//$'\n'/}" | |
| printf '%s' "${value}" | |
| } | |
| sanitize_control_plane_name() { | |
| local value="$1" | |
| value="${value//$'\r'/}" | |
| value="${value//$'\n'/}" | |
| value="${value//$'\t'/}" | |
| # strip leading/trailing spaces | |
| value="${value#"${value%%[! ]*}"}" | |
| value="${value%"${value##*[! ]}"}" | |
| printf '%s' "${value}" | |
| } |
| local label="$1" | ||
| local value="$2" | ||
|
|
||
| if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then |
There was a problem hiding this comment.
The current pattern allows consecutive hyphens (e.g. my--org), which DNS labels and most cloud providers prohibit. Consider tightening to reject them:
| if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then | |
| if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9]*(-[a-z0-9]+)*)?$ ]]; then |
This rewrites the middle section as zero-or-more runs of alphanumerics optionally separated by single hyphens, so my-org passes but my--org doesn't.
| local value="$2" | ||
|
|
||
| if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then | ||
| echo "::error::${label} must be a valid Control Plane org name; remove whitespace, line endings, or unsupported characters." |
There was a problem hiding this comment.
The error message doesn't show the invalid value. Since org names are vars.* (not secrets), printing the sanitized string here would make debugging much faster — the user can immediately see whether the issue is residual spaces, uppercase letters, etc.:
| echo "::error::${label} must be a valid Control Plane org name; remove whitespace, line endings, or unsupported characters." | |
| echo "::error::${label} ('${value}') must be a valid Control Plane org name (lowercase alphanumeric and hyphens, no consecutive hyphens); remove any whitespace, line endings, uppercase letters, or unsupported characters." |
Greptile SummaryThis PR fixes a Docker auth failure caused by CR/LF characters being stored in the
Confidence Score: 4/5Safe to merge; the normalization step is positioned after the existing non-empty validation and before all usages, so no step can receive a CR/LF-tainted org name. The fix is well-structured and covers all eight downstream consumers. The only gap is that the sanitizer strips CR/LF but not spaces or tabs, which the validator would catch and reject rather than auto-fix — a slight asymmetry that could confuse operators who pasted a trailing space instead of a newline. .github/workflows/cpflow-promote-staging-to-production.yml — specifically the sanitize_control_plane_name function and the validate_control_plane_org error message. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate-required-config\nraw vars.CPLN_ORG_*] --> B[Normalize Control Plane org names\nstrip CR/LF → validate regex]
B -->|validation fails| FAIL[❌ exit 1 with error]
B -->|steps.cpln-orgs.outputs.staging/production| C[Setup production environment]
B --> D[Verify production env vars]
B --> E[Capture current production image]
B --> F[Capture deployed staging image]
B --> G[Copy image from staging]
B --> H[Deploy image to production]
B --> I[Wait for deployment health]
H --> I
I -->|failure| J[Roll back on failure]
J --> K[Wait for rollback readiness]
B --> J
B --> K
Reviews (1): Last reviewed commit: "Sanitize Control Plane org names in prod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sanitize_control_plane_name() { | ||
| local value="$1" | ||
| value="${value//$'\r'/}" | ||
| value="${value//$'\n'/}" | ||
| printf '%s' "${value}" | ||
| } |
There was a problem hiding this comment.
sanitize_control_plane_name strips only CR/LF, not spaces or tabs
The function removes \r and \n, which handles the documented copy-paste artifact. However, spaces and tabs (also common in pasted values) are passed through unchanged. The downstream validate_control_plane_org regex will then reject them with a message that says "remove whitespace, line endings, or unsupported characters" — which is correct — but the value won't actually be fixed by the sanitizer. Extending the sanitizer to also strip horizontal whitespace would make the fix symmetric and reduce operator confusion when someone copies with a trailing space instead of a newline.
| sanitize_control_plane_name() { | |
| local value="$1" | |
| value="${value//$'\r'/}" | |
| value="${value//$'\n'/}" | |
| printf '%s' "${value}" | |
| } | |
| sanitize_control_plane_name() { | |
| local value="$1" | |
| value="${value//$'\r'/}" | |
| value="${value//$'\n'/}" | |
| value="${value//$' '/}" | |
| value="${value//$'\t'/}" | |
| printf '%s' "${value}" | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| validate_control_plane_org() { | ||
| local label="$1" | ||
| local value="$2" | ||
|
|
||
| if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then | ||
| echo "::error::${label} must be a valid Control Plane org name; remove whitespace, line endings, or unsupported characters." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The validate_control_plane_org error message names only the variable label, not the sanitized value being rejected. If validation fails for a reason other than trailing CR/LF (e.g. an upper-case letter, a leading dash, or a space that wasn't stripped), an operator looking at the Actions log won't be able to see the actual problematic value without inspecting the repository variable store manually. Including the sanitized value in the error message would halve the investigation round-trip.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
justin808
force-pushed
the
jg-codex/trim-control-plane-org-vars
branch
from
7975fee to
14533bd
Compare
|
Code Review - Solid fix for a real production failure. The normalization step correctly handles the documented root cause (trailing CR/LF on a copy-pasted repository variable) and routes all downstream steps through sanitized outputs. Low risk, CI-only change with no effect on application runtime. See inline comments for specific notes. |
| value="${value%"${value##*[![:space:]]}"}" | ||
|
|
||
| if [[ "${value}" == *$'\r'* || "${value}" == *$'\n'* ]]; then | ||
| echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization." >&2 |
There was a problem hiding this comment.
Minor inconsistency: this error message is sent to stderr (>&2) while validate_control_plane_org's error (line 143) goes to stdout. GitHub Actions processes ::error:: workflow commands from both streams, so both create annotations — but stdout is the conventional channel. Suggest aligning by dropping >&2 here:
| echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization." >&2 | |
| echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization." |
| value="${value#"${value%%[![:space:]]*}"}" | ||
| value="${value%"${value##*[![:space:]]}"}" |
There was a problem hiding this comment.
These lines implement a pure-bash leading/trailing whitespace trim via nested parameter expansion — a correct but fairly cryptic idiom. The logic is:
- Line 125:
${value%%[![:space:]]*}strips everything from the first non-space char to the end, leaving just the leading whitespace prefix; then${value#...}removes that prefix. - Line 126: mirror operation for the trailing side.
This is intentionally fork-free, but a future maintainer is likely to stumble on it. A short comment explaining the intent (pure-bash trim, avoids subprocesses) would help. Alternatively, value="$(printf '%s' "${value}" | sed -E 's/^[[:space:]]+|[[:space:]]+$//g')" is more readable at the cost of one sed subprocess — acceptable here since this step already runs bash.
| value="${value#"${value%%[![:space:]]*}"}" | |
| value="${value%"${value##*[![:space:]]}"}" | |
| # Pure-bash leading/trailing whitespace strip (avoids a subprocess). | |
| value="${value#"${value%%[![:space:]]*}"}" | |
| value="${value%"${value##*[![:space:]]}"}" |
✅ Review App DeletedReview app for PR #759 is deleted |
1 participant
Summary
Why
The latest production promote run failed in Docker auth with an invalid registry host:
shakacode-open-source-examples-production\r\n.registry.cpln.io.gh variable listconfirmsCPLN_ORG_PRODUCTIONis currently stored asshakacode-open-source-examples-production\r\n, so the workflow needs to tolerate copied/pasted line endings and fail early for truly invalid org names.Failed run: https://github.com/shakacode/react-webpack-rails-tutorial/actions/runs/26849977870
Validation
git diff --check -- .github/workflows/cpflow-promote-staging-to-production.ymlbin/conductor-exec bin/test-cpflow-github-flowNote
Low Risk
CI-only guardrails on repository variables; no application runtime or auth logic changes.
Overview
Adds a Normalize Control Plane org names step to the staging→production promotion workflow so
CPLN_ORG_STAGINGandCPLN_ORG_PRODUCTIONare trimmed, rejected if they still contain embedded line breaks, and checked against Control Plane’s org naming rules before promotion continues.All later steps that talk to Control Plane or Docker registries (setup, env parity, image capture/copy, deploy, health check, rollback) now use
steps.cpln-orgs.outputsinstead of raw repository variables, which avoids invalid hosts likeorg\r\n.registry.cpln.iowhen a variable was pasted with trailing CR/LF.Reviewed by Cursor Bugbot for commit 14533bd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit