ROSAENG-61732 | feat: Add create-time delete protection to rosa create cluster#3367
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughCluster creation supports Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
pkg/ocm/clusters.go (1)
790-803: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a doc comment and align the parameter name with the sibling method.
GetClusterDeletionProtectionis a new exported method without a doc comment, and its parameter is namedclusterIDwhile the adjacentUpdateClusterDeletionProtectionusesclusterIdfor the same concept.As per coding guidelines, "Use exported symbol doc comments when new public types or functions are introduced" and "Keep variable names explicit and consistent with nearby code."📝 Proposed fix
+// GetClusterDeletionProtection retrieves the delete-protection state for the given cluster. -func (c *Client) GetClusterDeletionProtection(clusterID string) (bool, error) { +func (c *Client) GetClusterDeletionProtection(clusterId string) (bool, error) { response, err := c.ocm.ClustersMgmt().V1().Clusters(). - Cluster(clusterID). + Cluster(clusterId). DeleteProtection(). Get(). Send()🤖 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 `@pkg/ocm/clusters.go` around lines 790 - 803, Add a Go doc comment for the exported Client.GetClusterDeletionProtection method, and rename its clusterID parameter to clusterId to match UpdateClusterDeletionProtection and nearby code while preserving the existing behavior.Source: Coding guidelines
pkg/ocm/delete_protection_test.go (1)
52-63: 🎯 Functional Correctness | 🔵 TrivialConsider covering the error and empty-body branches too.
Only the "enabled" happy path is tested.
GetClusterDeletionProtectionalso has an API-error branch and a nil-body branch (pkg/ocm/clusters.golines 796-801) that aren't exercised here.🤖 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 `@pkg/ocm/delete_protection_test.go` around lines 52 - 63, Extend the GetClusterDeletionProtection tests in delete_protection_test.go to cover both an API error response and a successful response with an empty or nil body. Assert that the API-error case returns an error and that the nil-body branch produces the expected result, reusing the existing apiServer and response helpers.
🤖 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 `@cmd/create/cluster/cmd.go`:
- Around line 3685-3700: In the enableDeleteProtection flow, replace the
os.Exit(1) after UpdateClusterDeletionProtection fails with a warning via
r.Reporter, then continue execution so cluster description and STS/OIDC
follow-up instructions are still produced. Keep the existing fatal handling for
delete-protection builder errors unchanged.
In `@pkg/ocm/delete_protection_test.go`:
- Line 60: Extend the tests around GetClusterDeletionProtection to cover both
response.Error() failures and responses where response.Body() is nil. Assert the
expected error behavior for each case while preserving the existing
successful-path coverage.
---
Nitpick comments:
In `@pkg/ocm/clusters.go`:
- Around line 790-803: Add a Go doc comment for the exported
Client.GetClusterDeletionProtection method, and rename its clusterID parameter
to clusterId to match UpdateClusterDeletionProtection and nearby code while
preserving the existing behavior.
In `@pkg/ocm/delete_protection_test.go`:
- Around line 52-63: Extend the GetClusterDeletionProtection tests in
delete_protection_test.go to cover both an API error response and a successful
response with an empty or nil body. Assert that the API-error case returns an
error and that the nil-body branch produces the expected result, reusing the
existing apiServer and response helpers.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 801dfa40-3eb1-4c1a-8579-891811bda633
📒 Files selected for processing (5)
cmd/create/cluster/cmd.gocmd/create/cluster/cmd_test.gocmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.ymlpkg/ocm/clusters.gopkg/ocm/delete_protection_test.go
02324ea to
8d6abea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/create/cluster/cmd.go (1)
3685-3699: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBuild-error branch still aborts the whole post-creation flow; only the update-error branch was fixed.
The
UpdateClusterDeletionProtectionfailure now correctly warns and continues (matching the earlier review feedback), but theBuild()failure branch above it (lines 3687-3690) still callsos.Exit(1)after the cluster has already been created — this loses the cluster-describe output and STS/OIDC follow-up instructions, the exact issue previously raised for this block. SinceEnabled(true).Build()has no required fields, this path is unlikely to fail, but for consistency it should warn instead of exit too.🔧 Proposed fix
if enableDeleteProtection { deleteProtection, err := v1.NewDeleteProtection().Enabled(true).Build() if err != nil { - r.Reporter.Errorf("Failed to build delete protection: %v", err) - os.Exit(1) - } - if err := r.OCMClient.UpdateClusterDeletionProtection(cluster.ID(), deleteProtection); err != nil { - r.Reporter.Warnf( - "Cluster '%s' was created but delete protection could not be enabled: %v", - cluster.ID(), - err, - ) + r.Reporter.Warnf("Cluster '%s' was created but delete protection could not be enabled: %v. "+ + "You can enable it later with 'rosa edit cluster'.", cluster.ID(), err) + } else if err := r.OCMClient.UpdateClusterDeletionProtection(cluster.ID(), deleteProtection); err != nil { + r.Reporter.Warnf( + "Cluster '%s' was created but delete protection could not be enabled: %v", + cluster.ID(), + err, + ) } }🤖 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 `@cmd/create/cluster/cmd.go` around lines 3685 - 3699, Update the `v1.NewDeleteProtection().Enabled(true).Build()` error branch in the `enableDeleteProtection` flow to report a warning and continue instead of calling `os.Exit(1)`. Preserve the existing warning-and-continue behavior for `UpdateClusterDeletionProtection`, allowing the post-creation cluster description and follow-up instructions to run.
🧹 Nitpick comments (1)
cmd/dlt/cluster/cmd_test.go (1)
151-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
GetClusterDeletionProtectionreturning an error.Only the enabled/disabled branches are tested; the branch where the OCM call itself fails (line 141 in cmd.go) is untested, even though it now short-circuits cluster deletion.
As per coding guidelines, "Add focused automated tests when behavior changes in a way that could regress."
🤖 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 `@cmd/dlt/cluster/cmd_test.go` around lines 151 - 179, Add a focused test in the ensureDeleteProtectionDisabled context covering GetClusterDeletionProtection returning an error from the API call. Configure the mock server to return a failure, invoke ensureDeleteProtectionDisabled, and assert that the error is propagated and cluster deletion is short-circuited without treating protection as enabled or disabled.Source: Coding guidelines
🤖 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 `@pkg/ocm/delete_protection_test.go`:
- Around line 74-82: The test currently covers an empty JSON object rather than
a nil response body. Update the test around GetClusterDeletionProtection to
construct or return a response with Body() equal to nil, then assert the call
returns the “delete protection response body is empty” error and the expected
disabled value.
In `@tests/e2e/test_rosacli_cluster.go`:
- Around line 463-464: Update both ContainSubstring assertions in the relevant
ROSA CLI test to use the exact lowercase text “delete protection is active,”
including the assertion that currently checks the capitalized variant; leave the
--enable-delete-protection=false assertion unchanged.
---
Duplicate comments:
In `@cmd/create/cluster/cmd.go`:
- Around line 3685-3699: Update the
`v1.NewDeleteProtection().Enabled(true).Build()` error branch in the
`enableDeleteProtection` flow to report a warning and continue instead of
calling `os.Exit(1)`. Preserve the existing warning-and-continue behavior for
`UpdateClusterDeletionProtection`, allowing the post-creation cluster
description and follow-up instructions to run.
---
Nitpick comments:
In `@cmd/dlt/cluster/cmd_test.go`:
- Around line 151-179: Add a focused test in the ensureDeleteProtectionDisabled
context covering GetClusterDeletionProtection returning an error from the API
call. Configure the mock server to return a failure, invoke
ensureDeleteProtectionDisabled, and assert that the error is propagated and
cluster deletion is short-circuited without treating protection as enabled or
disabled.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b417cb43-0e17-4b61-a20c-1b7366bedb3b
📒 Files selected for processing (10)
cmd/create/cluster/cmd.gocmd/create/cluster/cmd_test.gocmd/dlt/cluster/cmd.gocmd/dlt/cluster/cmd_test.gocmd/edit/cluster/cmd.gocmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.ymlpkg/ocm/clusters.gopkg/ocm/delete_protection_test.gotests/e2e/test_rosacli_cluster.gotests/e2e/test_rosacli_cluster_post.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.yml
- cmd/create/cluster/cmd_test.go
- pkg/ocm/clusters.go
8d6abea to
8b03f1a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/create/cluster/cmd.go (1)
3686-3695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid shadowing
errin the post-creation branch.Line 3686 shadows the outer
err, and Line 3691 shadows it again. Use distinct names such asbuildErrandupdateErrto follow the Go guidance and keep the two failure paths unambiguous.Proposed fix
- deleteProtection, err := v1.NewDeleteProtection().Enabled(true).Build() - if err != nil { - r.Reporter.Errorf("Failed to build delete protection: %v", err) + deleteProtection, buildErr := v1.NewDeleteProtection().Enabled(true).Build() + if buildErr != nil { + r.Reporter.Errorf("Failed to build delete protection: %v", buildErr) os.Exit(1) } - if err := r.OCMClient.UpdateClusterDeletionProtection(cluster.ID(), deleteProtection); err != nil { + if updateErr := r.OCMClient.UpdateClusterDeletionProtection(cluster.ID(), deleteProtection); updateErr != nil { r.Reporter.Warnf( "Cluster '%s' was created but delete protection could not be enabled: %v", cluster.ID(), - err, + updateErr, ) }🤖 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 `@cmd/create/cluster/cmd.go` around lines 3686 - 3695, Rename the inner error variables in the post-creation delete-protection flow to avoid shadowing the outer err: use distinct names such as buildErr for NewDeleteProtection().Enabled(true).Build() and updateErr for UpdateClusterDeletionProtection. Update each corresponding error check and log reference while preserving the existing failure behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/create/cluster/cmd.go`:
- Around line 3686-3695: Rename the inner error variables in the post-creation
delete-protection flow to avoid shadowing the outer err: use distinct names such
as buildErr for NewDeleteProtection().Enabled(true).Build() and updateErr for
UpdateClusterDeletionProtection. Update each corresponding error check and log
reference while preserving the existing failure behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cf58033b-9a1c-46e6-9ade-1d6f7ba6a271
📒 Files selected for processing (10)
cmd/create/cluster/cmd.gocmd/create/cluster/cmd_test.gocmd/dlt/cluster/cmd.gocmd/dlt/cluster/cmd_test.gocmd/edit/cluster/cmd.gocmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.ymlpkg/ocm/clusters.gopkg/ocm/delete_protection_test.gotests/e2e/test_rosacli_cluster.gotests/e2e/test_rosacli_cluster_post.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.yml
- cmd/dlt/cluster/cmd_test.go
- cmd/edit/cluster/cmd.go
- cmd/create/cluster/cmd_test.go
- pkg/ocm/delete_protection_test.go
- tests/e2e/test_rosacli_cluster_post.go
- cmd/dlt/cluster/cmd.go
- tests/e2e/test_rosacli_cluster.go
8b03f1a to
83784d2
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
83784d2 to
5d75dee
Compare
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 `@tests/e2e/test_rosacli_cluster.go`:
- Around line 463-464: Update the delete-protection error assertions in the
current test and the Day1 delete-protection test to match the CLI message’s
punctuation, placing the comma after the cluster ID rather than directly after
“active”. Keep the existing --enable-delete-protection=false assertion
unchanged.
- Around line 1919-1925: Update both delete-protection assertions in
tests/e2e/test_rosacli_cluster.go at lines 1919-1925 and 463-464 to match the
CLI wording by asserting the substring “delete protection is active on cluster”;
leave the existing guidance assertion unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3e2fbc84-6416-4c9d-b6b9-74da003f43d7
📒 Files selected for processing (10)
cmd/create/cluster/cmd.gocmd/create/cluster/cmd_test.gocmd/dlt/cluster/cmd.gocmd/dlt/cluster/cmd_test.gocmd/edit/cluster/cmd.gocmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.ymlpkg/ocm/clusters.gopkg/ocm/delete_protection_test.gotests/e2e/test_rosacli_cluster.gotests/e2e/test_rosacli_cluster_post.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.yml
- pkg/ocm/clusters.go
- cmd/edit/cluster/cmd.go
- cmd/create/cluster/cmd_test.go
- tests/e2e/test_rosacli_cluster_post.go
- cmd/dlt/cluster/cmd.go
- pkg/ocm/delete_protection_test.go
- cmd/create/cluster/cmd.go
5d75dee to
bdd2f94
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
bdd2f94 to
de4a9e4
Compare
de4a9e4 to
fd3ad43
Compare
|
Lint seems to be failing due to issues unrelated to this PR. |
fd3ad43 to
1cc42ad
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@olucasfreitas I think the lint failure is related to this PR #3345 that touched files from before the new-from-rev, raising old issues. Will the contributor submit a new one for fixing lint? |
|
Yay! All e2es passed, ready for final review and merge @olucasfreitas @amandahla. |
|
@michaelryanmcneill on my side, all good, I left a minor comment regarding adding a test. |
|
/approve |
openshift-ci
Bot
added
the
approved
…e cluster Enable --enable-delete-protection on cluster create (Classic and HCP) via post-create OCM PATCH, matching the existing edit-cluster flag. Add GetClusterDeletionProtection to pkg/ocm for live sub-resource GET. Signed-off-by: michaelryanmcneill <michael@michaelryanmcneill.com>
1cc42ad to
918911c
Compare
|
All fixed @olucasfreitas @amandahla! Once the lint issue is fixed, I'll rebase and then we can merge. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, michaelryanmcneill 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 |
28619c9
into
openshift:master
|
@michaelryanmcneill: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
PR Summary
Add create-time
--enable-delete-protectiontorosa create cluster, fast-failrosa delete clusterwhen protection is active (with an actionable disable command), clearer create/edit flag help text, and e2e coverage mirroring the existing edit-path tests.Detailed Description of the Issue
ROSA supports OCM delete protection via the
/delete_protectionsub-resource. The CLI already exposes day-2 toggling throughrosa edit cluster --enable-delete-protection, but:--enable-delete-protection=falsedisables protection on edit.Delete protection is not part of the cluster POST body; the correct create pattern is POST cluster → PATCH delete_protection.
Related Issues and PRs
delete_protectionon cluster resources (required provider version)PATCH/GET /api/clusters_mgmt/v1/clusters/{id}/delete_protectionType of Change
Previous Behavior
rosa create clusterhad no delete protection flag.rosa edit cluster --enable-delete-protection.rosa delete clusterrelied on the OCM DELETE API error when protection was enabled (no local check).Behavior After This Change
Create (
rosa create cluster)--enable-delete-protectionenables OCM delete protection immediately after successful cluster creation (Classic and HCP).false).UpdateClusterDeletionProtection; create fails if PATCH fails, with cluster ID in the error (cluster already exists).ocm.Spec— sub-resource only.buildCommand()replay includes--enable-delete-protectionwhen set (including interactive choice synced back toargs).Edit (
rosa edit cluster)--enable-delete-protection=false(no new flag).Delete (
rosa delete cluster)GetClusterDeletionProtectionbefore promptingconfirm.Confirm("delete cluster …"), so users are not asked to confirm a delete that cannot proceed.Library
GetClusterDeletionProtection(clusterID)added inpkg/ocm/clusters.gofor live sub-resource GET.How to Test (Step-by-Step)
Preconditions
make rosa).Test Steps
--enable-delete-protection, describe, attempt delete (expect fast-fail with disable command), then disable and delete.[id:73162][id:74657][id:73163]Expected Results
=falseto disable.cmd/dlt/cluster.rosa edit cluster -c … --enable-delete-protection=false.Proof of the Fix
Screenshots: N/A
Videos: N/A
Logs/CLI output:
Other artifacts:
cmd/rosa/structure_test/command_args/rosa/create/cluster/command_args.ymlBreaking Changes
Breaking Change Details / Migration Plan
N/A — new optional create flag (default off); delete now fails earlier with a clearer message when protection is already active (same outcome as API rejection). Help text wording only for create/edit flags.
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Additional Notes
After discussion, we've decided to fail in the event that the PATCH to the cluster deletion does not succeed. This means that the user won't see the "cluster created" message or STS cleanup commands if the error fires. This is intended to prevent automation silent failures which could result in a cluster being created without the intended deletion protection.
Summary by CodeRabbit
Summary by CodeRabbit
--enable-delete-protectiontorosa create cluster, including interactive prompting and--run againsupport; when enabled, delete protection is applied after creation (non-dry-run).rosa delete clusternow checks delete protection upfront and exits early with clearer guidance when it’s enabled.--enable-delete-protectionhelp text and aligned prompt wording to “delete protection.”