OSAC-1586: add DELETING and DELETE_FAILED states to ClusterState enum - #913
Conversation
|
@vladikr: This pull request references OSAC-1586 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
WalkthroughThe private and public ChangesCluster lifecycle states
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ 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 |
d7ed9fc to
c3ded20
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/public/osac/public/v1/cluster_type.proto`:
- Around line 284-288: Update the CLUSTER_STATE_DELETE_FAILED documentation to
reference the existing error-bearing field in ClusterStatus instead of
status.message. Use the actual field name exposed by ClusterStatus, or remove
the field reference if no suitable field exists; do not introduce a new message
field as part of this documentation change.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1baf93e5-aafb-4e0a-8d50-0f99bef271c8
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/private/osac/private/v1/cluster_type.protoproto/public/osac/public/v1/cluster_type.proto
| // The cluster deletion has failed. | ||
| // | ||
| // The deprovision operation encountered an error and could not complete. | ||
| // The status.message field will contain specific error details. | ||
| CLUSTER_STATE_DELETE_FAILED = 5; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document an existing error field instead of status.message.
ClusterStatus has no message field, so clients cannot find the deletion error details promised here. Reference an existing error-bearing field, or add message as a separate API contract change.
🤖 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 `@proto/public/osac/public/v1/cluster_type.proto` around lines 284 - 288,
Update the CLUSTER_STATE_DELETE_FAILED documentation to reference the existing
error-bearing field in ClusterStatus instead of status.message. Use the actual
field name exposed by ClusterStatus, or remove the field reference if no
suitable field exists; do not introduce a new message field as part of this
documentation change.
Add CLUSTER_STATE_DELETING (4) and CLUSTER_STATE_DELETE_FAILED (5) to the ClusterState proto enum in both private and public APIs, matching the convention used by all other OSAC resource types. This enables the osac-operator FeedbackReconciler to report cluster deletion state to the fulfillment-service. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
ClusterStatus has no top-level message field; error details are in the status conditions. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
c3ded20 to
83b75e9
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sk-ilya, vladikr 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 |
|
Thanks @sk-ilya ! |
This branch was previously deployed
3 participants
Summary
CLUSTER_STATE_DELETING(4) andCLUSTER_STATE_DELETE_FAILED(5) to theClusterStateproto enum in both private and public APIsTest plan
buf lintpassesbuf generatesucceedsginkgo run -r internal)Companion PR: osac-project/osac-operator (pending — implements the FeedbackReconciler changes that consume these new states)
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit