HYPERFLEET-1289 - feat: tag e2e namespaces with hyperfleet.io/test-ru…#61
HYPERFLEET-1289 - feat: tag e2e namespaces with hyperfleet.io/test-ru…#61rh-amarin wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
🤖 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 `@helm/namespace-cleaner/scripts/namespace-cleaner.sh`:
- Around line 75-99: The namespace cleanup loop in namespace-cleaner.sh
processes the same namespace multiple times when it matches more than one
selector, which leads to duplicate output and misleading delete warnings. Add
cross-iteration deduplication in the main selector loop by tracking namespace
names before the delete path, and skip any namespace already seen so each
namespace is handled only once regardless of how many selectors match it.
In `@Makefile`:
- Around line 337-340: The check-hyperfleet-namespace Make target is expanding
NAMESPACE directly into shell commands, which allows command injection when
overridden from the CLI or environment. Update the recipe to validate NAMESPACE
once before any shell invocation (reuse or strengthen check-namespace if
needed), then pass it safely quoted to the kubectl label command and the echo
output. Keep the fix localized to the check-hyperfleet-namespace target and its
supporting namespace validation logic.
🪄 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: CHILL
Plan: Enterprise
Run ID: 9755fa33-5af1-4506-a03d-d6bd13a047fe
📒 Files selected for processing (3)
Makefilehelm/namespace-cleaner/scripts/namespace-cleaner.shhelm/namespace-cleaner/values.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
rh-amarin
force-pushed
the
tag-e2e-namespace
branch
2 times, most recently
from
ad084bf to
7ae3ab3
Compare
…n and clean up via namespace cleaner Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
force-pushed
the
tag-e2e-namespace
branch
from
7ae3ab3 to
a4c7e0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@helm/namespace-cleaner/templates/_helpers.tpl`:
- Around line 47-48: The cluster-scoped name helper can still collide after
truncation because `namespace-cleaner.clusterResourceName` truncates the entire
combined fullname and namespace string, which can drop the namespace identifier
entirely. Update this helper so the namespace contribution is preserved as a
fixed-length suffix derived from `.Release.Namespace` while keeping the base
`namespace-cleaner.fullname` truncated to fit within the 63-character limit.
Ensure the resulting name remains unique per namespace even when the fullname is
long.
🪄 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: CHILL
Plan: Enterprise
Run ID: bf032c49-457d-45c4-82dd-113ee92a5a64
📒 Files selected for processing (6)
Makefilehelm/namespace-cleaner/scripts/namespace-cleaner.shhelm/namespace-cleaner/templates/_helpers.tplhelm/namespace-cleaner/templates/clusterrole.yamlhelm/namespace-cleaner/templates/clusterrolebinding.yamlhelm/namespace-cleaner/values.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/namespace-cleaner/values.yaml
- helm/namespace-cleaner/scripts/namespace-cleaner.sh
- Makefile
| {{- define "namespace-cleaner.clusterResourceName" -}} | ||
| {{- printf "%s-%s" (include "namespace-cleaner.fullname" .) .Release.Namespace | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve namespace uniqueness after truncation.
This helper can still generate colliding cluster-scoped names when namespace-cleaner.fullname already consumes most of the 63-character budget, because .Release.Namespace gets truncated off. That reintroduces ClusterRole/ClusterRoleBinding conflicts between installs in different namespaces and breaks the isolation this PR is adding. Keep a fixed-length namespace-derived suffix instead of truncating the entire combined string.
🤖 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 `@helm/namespace-cleaner/templates/_helpers.tpl` around lines 47 - 48, The
cluster-scoped name helper can still collide after truncation because
`namespace-cleaner.clusterResourceName` truncates the entire combined fullname
and namespace string, which can drop the namespace identifier entirely. Update
this helper so the namespace contribution is preserved as a fixed-length suffix
derived from `.Release.Namespace` while keeping the base
`namespace-cleaner.fullname` truncated to fit within the 63-character limit.
Ensure the resulting name remains unique per namespace even when the fullname is
long.
| echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [DRY-RUN] Would delete namespace '${ns_name}' (age=${age_m}m)" | ||
| else | ||
| echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [INFO] Deleting namespace '${ns_name}' (age=${age_m}m)" | ||
| kubectl delete namespace "${ns_name}" --wait=false \ |
There was a problem hiding this comment.
Since this is a non-blocking call, it won't error out if the namespace failed to be deleted. With --wait=false, kubectl would almost never fail because it only validates that the DELETE request was accepted by the API server.
I'm not sure what the alternative is because we don't want to block the deletion. But the namespace could just hang in terminating state forever and the logs from this script would be "Delete requested for namespace"
2 participants
Summary
make install-hyperfleetnow labels the target namespace withhyperfleet.io/test-run=<namespace-name>before helmfile runshyperfleet.io/test-run(OR semantics — each selector queried independently via a for loop)