fix(setup): make Temporal namespace creation idempotent via describe#3679
Open
shayan1995 wants to merge 1 commit into
Open
fix(setup): make Temporal namespace creation idempotent via describe#3679shayan1995 wants to merge 1 commit into
shayan1995 wants to merge 1 commit into
Conversation
Most of NVIDIA#2768 (error suppression, missing diagnostics) was already addressed on main by NVIDIA#2546 and NVIDIA#2675: namespace create output is captured, "already exists" is tolerated, genuine errors are propagated, and _verify_temporal_namespaces double-checks the result. This closes the remaining gap from the issue's expected behaviour: check namespace existence with 'temporal operator namespace describe' and skip creation when it already exists, instead of relying on the create call failing with "already exists". Any describe failure (not-found or transient) simply falls through to create, which already propagates real errors (TLS, auth, connectivity) with diagnostics. Fixes NVIDIA#2768 Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe Temporal namespace setup now checks whether the target namespace exists before creating it. Existing namespaces return through an idempotent fast path, while other outcomes continue to the existing creation and error-handling logic. ChangesTemporal namespace setup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/ok to test dcfef03 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Temporal namespace creation in
helm-prereqs/setup.shused to suppress all errors (2>/dev/null || true), which #2768 flagged as masking TLS/auth/connectivity faults. Most of that was since fixed on main (#2546, #2675): create output is captured, "already exists" is tolerated, genuine errors propagate, and_verify_temporal_namespacesdouble-checks the result.This PR closes the remaining gap from the issue's expected behaviour:
_create_temporal_namespacenow checks existence withtemporal operator namespace describeand skips creation when the namespace already exists, instead of relying on the create call failing with "already exists". Any describe failure (not-found or transient) simply falls through to create, which already propagates real errors with diagnostics — so no new error-message parsing is introduced.Related issues
Fixes #2768
Type of Change
Breaking Changes
Testing
bash -nclean. Behavioural safety net is unchanged: even if the describe fast-path misjudges, the existing create path and_verify_temporal_namespacesstill catch and report real failures. (TODO before submitting: run setup.sh phase 7f against a cluster twice to confirm the skip path logs "already exists".)