-
Notifications
You must be signed in to change notification settings - Fork 82
✨ Add work progressing condition reasons #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add work progressing condition reasons #398
Conversation
Signed-off-by: Qing Hao <[email protected]>
WalkthroughAdds three new exported progressing reason constants—Applying, Completed, and Failed—to work/v1 types, within the WorkManifestsComplete const group, expanding the set of manifest work status reasoning values. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
work/v1/types.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit
- GitHub Check: verify
// WorkProgressingReasonApplying indicates resources are being applied | ||
WorkProgressingReasonApplying string = "Applying" | ||
// WorkProgressingReasonCompleted indicates all resources are applied and available | ||
WorkProgressingReasonCompleted string = "Completed" | ||
// WorkProgressingReasonFailed indicates the work failed to apply | ||
WorkProgressingReasonFailed string = "Failed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Well-documented constants with clear semantic intent.
The three new progressing reason constants are well-named and clearly documented. The string values ("Applying", "Completed", "Failed") provide good semantic clarity for tracking work progression states.
However, verify the following:
- Ensure these constants are used in the codebase to avoid introducing dead code.
- Clarify the distinction between
WorkProgressingReasonCompleted
(line 523) andWorkComplete
(line 509). While one is a reason value and the other is a condition type, the similar naming could cause confusion for API consumers.
Run the following script to verify usage of these new constants:
🏁 Script executed:
#!/bin/bash
# Description: Verify that the new progressing reason constants are used in the codebase
echo "Searching for WorkProgressingReasonApplying usage:"
rg -n "WorkProgressingReasonApplying" --type go
echo -e "\nSearching for WorkProgressingReasonCompleted usage:"
rg -n "WorkProgressingReasonCompleted" --type go
echo -e "\nSearching for WorkProgressingReasonFailed usage:"
rg -n "WorkProgressingReasonFailed" --type go
echo -e "\nSearching for raw string literals that might be using these reasons:"
rg -n '"(Applying|Completed|Failed)"' --type go -g '!**/types.go' -C 3
Length of output: 2377
Implement or remove unused progressing reasons
WorkProgressingReasonApplying, WorkProgressingReasonCompleted, and WorkProgressingReasonFailed (work/v1/types.go:520–525) have no references outside their own definitions. Wire them into the work status transitions where appropriate or remove to avoid dead code.
🤖 Prompt for AI Agents
In work/v1/types.go around lines 520–525, the three WorkProgressingReason*
constants are defined but unused; either remove them or wire them into the work
status transition logic. To fix: search the reconciler(s) and status update code
that set Work.Status/conditions for progressing/completed/failed and replace
hard-coded reason strings (or add missing reason values) to use these constants,
update any helper functions that construct Condition objects to reference
WorkProgressingReasonApplying, WorkProgressingReasonCompleted, and
WorkProgressingReasonFailed, and add/adjust unit tests to assert the correct
reason is set; alternatively, if these reasons are not needed, delete the unused
constants and any related dead code/comments. Ensure imports/compile and tests
pass after the change.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6b4f26d
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #open-cluster-management-io/ocm#1201
Summary by CodeRabbit