HYPERFLEET-839 - fix: require explicit NAMESPACE to prevent Pub/Sub c…#86
Conversation
📝 WalkthroughWalkthroughThe PR makes namespace usage per-user and stricter across deploy scripts and docs: deploy-scripts/.env.example now defaults NAMESPACE to Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy-scripts/README.md`:
- Around line 180-184: The README example for running
./deploy-scripts/deploy-clm.sh shows a broken multi-line shell command: the line
with "--namespace pr-123" is missing a trailing backslash, causing the next
option lines to be treated as a separate command; update the example so each
continued line ends with a backslash (e.g., add a "\" after "--namespace
pr-123") so the full invocation of deploy-clm.sh with --action, --namespace,
--api-image-repo and --api-image-tag is executed as one multi-line command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6699e988-d74b-4d63-87a0-2b50afe0694b
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy-scripts/.env.example`:
- Line 12: The NAMESPACE export uses unguarded $USER expansion which can trigger
an "unbound variable" under set -u; update the export for NAMESPACE (the line
that currently reads export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$USER}") to
use safe parameter expansion such as export
NAMESPACE="${NAMESPACE:-hyperfleet-e2e-${USER:-unknown}}" (or ${USER:-} if you
prefer empty) so that missing USER won’t abort deploy-clm.sh.
In `@deploy-scripts/deploy-clm.sh`:
- Around line 360-366: The script currently only checks NAMESPACE presence; add
DNS-1123 format validation (lowercase alphanumerics and hyphens, no
leading/trailing hyphen, max 63 chars) right after the existing empty check. In
the block around the NAMESPACE check, validate length (<=63) and match the regex
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$; if it fails, call log_error with a clear
message about invalid namespace format and then print_usage and exit 1 (same
flow as the empty-case). Reference: NAMESPACE variable and existing
log_error/print_usage usage in deploy-clm.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 357fd5ee-da38-4294-8f22-f05f7ff8e918
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
✅ Files skipped from review due to trivial changes (1)
- deploy-scripts/README.md
| ./deploy-scripts/deploy-clm.sh --action install \ | ||
| --namespace dev-test \ | ||
| --namespace <unique_namespace> |
There was a problem hiding this comment.
Missing trailing backslash. This breaks the multi-line shell example. The image repo and tag lines would be interpreted as a separate command instead of arguments to deploy-clm.sh.
| # If NAMESPACE is not set, uses a default value of hyperfleet-e2e-$USER | ||
| # Ideally, if you continue to see Pub/Sub conflicts, you can pass in the | ||
| # --namespace flag to the deploy-clm.sh script. | ||
| export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$USER}" |
There was a problem hiding this comment.
$USER can contain uppercase characters on some systems, which would produce an invalid Kubernetes namespace (DNS-1123 requires lowercase). Consider lowercasing it here or adding DNS-1123 validation in the script to catch this early.
There was a problem hiding this comment.
Addressed and added a check for namespace names that must be valid RFC 1123 DNS labels.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy-scripts/.env.example (1)
12-12: ⚡ Quick winSanitize and bound the
USERsuffix before composingNAMESPACE.Line 12 lowercases
USER, but it still allows characters/length that fail the DNS-1123 check indeploy-scripts/deploy-clm.sh(Line 368). This can make the default.envvalue fail on first run for valid system usernames likejohn_doeor long IDs.Suggested patch
-export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$(echo ${USER:-default} | tr '[:upper:]' '[:lower:]')}" +export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$(printf '%s' "${USER:-default}" | tr '[:upper:]' '[:lower:]' | sed -E 's/[^a-z0-9]+/-/g; s/^-+|-+$//g; s/^$/default/; s/^(.{48}).*$/\1/')}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy-scripts/.env.example` at line 12, Sanitize and bound the USER suffix used to build NAMESPACE: when exporting NAMESPACE (the export NAMESPACE= line), lower-case the USER, replace any chars not in [a-z0-9-] with '-', strip leading/trailing '-' and enforce a maximum suffix length so the full name (prefix "hyperfleet-e2e-") stays within the DNS-1123 63-char label limit (i.e., cap the suffix to 63 minus the prefix length); ensure an empty result falls back to "default"; apply this change in the .env.example export NAMESPACE line and verify it satisfies the DNS-1123 check used in deploy-clm.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deploy-scripts/.env.example`:
- Line 12: Sanitize and bound the USER suffix used to build NAMESPACE: when
exporting NAMESPACE (the export NAMESPACE= line), lower-case the USER, replace
any chars not in [a-z0-9-] with '-', strip leading/trailing '-' and enforce a
maximum suffix length so the full name (prefix "hyperfleet-e2e-") stays within
the DNS-1123 63-char label limit (i.e., cap the suffix to 63 minus the prefix
length); ensure an empty result falls back to "default"; apply this change in
the .env.example export NAMESPACE line and verify it satisfies the DNS-1123
check used in deploy-clm.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 85cd9844-f0f0-4082-9f38-9899c6b94688
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy-scripts/README.md
| 3. **Run the deployment:** | ||
| ```bash | ||
| ./deploy-clm.sh --action install | ||
| ./deploy-clm.sh --action install --namespace hyperfleet-e2e-$USER |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
Two small doc issues with the namespace examples:
-
The README mixes
<unique_namespace>(placeholder) andhyperfleet-e2e-$USER(literal) without a consistent rule. For example, Quick Start install uses the placeholder but Quick Start uninstall uses the literal. Consider standardizing — either use<unique_namespace>everywhere with a note explaining the convention, or usehyperfleet-e2e-$USEReverywhere for copy-pasteable examples. -
Option 2 step 3 (line 87) adds
--namespace hyperfleet-e2e-$USERto the .env-based workflow, but the whole point of Option 2 is that NAMESPACE is configured in.env. The--namespaceCLI flag overrides the.envvalue, making step 2's config irrelevant. The script's own help text shows the correct pattern:
# Install with .env defaults
deploy-clm.sh --action installConsider reverting line 87 to just ./deploy-clm.sh --action install (no --namespace) to match the .env-based intent.
Impact warningsThe following files are NOT in this PR but still reference the old hardcoded
These are outside the PR scope — just flagging for awareness so the migration away from the hardcoded namespace is complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy-scripts/README.md (1)
304-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one namespace variable consistently in the E2E snippet.
Line 309 uses
$NAMESPACE, but Lines 306 and 321 still use<unique_namespace>. This makes the snippet inconsistent and less copy/pasteable. DefineNAMESPACEonce and use it for deploy, lookup, and cleanup.Suggested snippet adjustment
# Deploy test environment -./deploy-scripts/deploy-clm.sh --action install --namespace <unique_namespace> +export NAMESPACE=<unique_namespace> +./deploy-scripts/deploy-clm.sh --action install --namespace $NAMESPACE # Configure E2E test API URL EXTERNAL_IP=$(kubectl get svc hyperfleet-api -n $NAMESPACE -o jsonpath='{.status.loadBalancer.ingress[0].ip}') export HYPERFLEET_API_URL="http://${EXTERNAL_IP}:8000" @@ -./deploy-scripts/deploy-clm.sh --action uninstall --namespace <unique_namespace> +./deploy-scripts/deploy-clm.sh --action uninstall --namespace $NAMESPACEAs per coding guidelines, "In any E2E integration steps, use $NAMESPACE when extracting services ... and when passing namespaces to deploy/cleanup."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy-scripts/README.md` around lines 304 - 322, The snippet is inconsistent about namespace usage—replace all literal "<unique_namespace>" occurrences with a single exported variable NAMESPACE and use it everywhere; explicitly export NAMESPACE at the top (e.g., export NAMESPACE="<unique_namespace>"), then call ./deploy-scripts/deploy-clm.sh --namespace "$NAMESPACE" for install and uninstall and use kubectl get svc hyperfleet-api -n "$NAMESPACE" to populate EXTERNAL_IP used in HYPERFLEET_API_URL so the deploy-clm.sh invocations, the kubectl lookup, and the cleanup all reference the same NAMESPACE variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy-scripts/deploy-clm.sh`:
- Around line 132-134: Update the help text in deploy-clm.sh that currently
recommends setting NAMESPACE=hyperfleet-e2e-$USER so it instead references the
canonical default in deploy-scripts/.env.example (which uses the lowercased user
expression) or suggests using the --namespace <unique_namespace> flag; modify
the NAMESPACE recommendation string and any related usage/help output in
deploy-clm.sh to match the .env.example behavior (lowercase user expression)
and/or mention the --namespace option to avoid validation mismatches.
---
Outside diff comments:
In `@deploy-scripts/README.md`:
- Around line 304-322: The snippet is inconsistent about namespace usage—replace
all literal "<unique_namespace>" occurrences with a single exported variable
NAMESPACE and use it everywhere; explicitly export NAMESPACE at the top (e.g.,
export NAMESPACE="<unique_namespace>"), then call ./deploy-scripts/deploy-clm.sh
--namespace "$NAMESPACE" for install and uninstall and use kubectl get svc
hyperfleet-api -n "$NAMESPACE" to populate EXTERNAL_IP used in
HYPERFLEET_API_URL so the deploy-clm.sh invocations, the kubectl lookup, and the
cleanup all reference the same NAMESPACE variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 285be350-01e1-4796-8b83-d373d36a5704
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
| NAMESPACE should be unique to prevent GCP Pub/Sub collisions. | ||
| Recommended: set NAMESPACE=hyperfleet-e2e-\$USER in .env file. | ||
|
|
There was a problem hiding this comment.
Align the help-text recommendation with the canonical .env default.
Line 133 recommends NAMESPACE=hyperfleet-e2e-$USER, which can drift from the .env.example behavior and lead users to values later rejected by validation. Please point users to the .env.example default expression (lowercased user) or to --namespace <unique_namespace>.
Suggested doc tweak
- Recommended: set NAMESPACE=hyperfleet-e2e-\$USER in .env file.
+ Recommended: use the default in .env.example (hyperfleet-e2e-<lowercased-user>)
+ or pass --namespace <unique_namespace>.As per coding guidelines, "deploy-scripts/.env.example ... should remain the canonical place for the default namespace behavior that deploy-clm.sh expects users to use."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy-scripts/deploy-clm.sh` around lines 132 - 134, Update the help text in
deploy-clm.sh that currently recommends setting NAMESPACE=hyperfleet-e2e-$USER
so it instead references the canonical default in deploy-scripts/.env.example
(which uses the lowercased user expression) or suggests using the --namespace
<unique_namespace> flag; modify the NAMESPACE recommendation string and any
related usage/help output in deploy-clm.sh to match the .env.example behavior
(lowercase user expression) and/or mention the --namespace option to avoid
validation mismatches.
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 3b927a6
into
openshift-hyperfleet:main
3 participants
Summary
Removes hardcoded default namespace of
hyperfleet-e2ein deploy-clm.sh to enforce that users must set NAMESPACE via .env or --namespace flag. This prevents GCP Pub/Sub topic/subscription collisions when multiple users run tests concurrently.Changes
Test Plan
deploy-clm.sh scripts successfulmake testpassesmake e2epassesmake lintpassesJira Issue