feat: tweak the condition behaviors in the new work applier #1039
Conversation
This file contains 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
2 tasks
ryanzhang-oss
approved these changes
Feb 14, 2025
| // Some exported reasons. Currently only the untrackable reason is being actively used. | ||
| WorkNotTrackableReason = string(ManifestProcessingAvailabilityResultTypeNotTrackable) | ||
| // Some exported reasons for Work object conditions. Currently only the untrackable reason is being actively used. | ||
| WorkNotAllManfestsTrackableReason = "SomeManifestsAreNotAvailabilityTrackable" |
There was a problem hiding this comment.
Suggested change
| WorkNotAllManfestsTrackableReason = "SomeManifestsAreNotAvailabilityTrackable" | |
| WorkNotAllManifestsTrackableReason = "SomeManifestsAreNotAvailabilityTrackable" |
Comment on lines
+151
to
+153
| if isManifestObjectDiffReported(bundle.reportDiffResTyp) { | ||
| diffReportedObjectsCount++ | ||
| } |
There was a problem hiding this comment.
Why Is there no UT test change related to this change that count "both diff and noDiff as reported"
| } | ||
|
|
||
| if work.Status.Conditions == nil { | ||
| work.Status.Conditions = []metav1.Condition{} | ||
| } | ||
| setWorkAppliedCondition(work, manifestCount, appliedManifestsCount) | ||
| setWorkAvailableCondition(work, manifestCount, availableAppliedObjectsCount, untrackableAppliedObjectsCount) | ||
| setWorkDiffReportedCondition(work, manifestCount, diffedObjectsCount) | ||
| setWorkAvailableCondition(work, manifestCount, appliedManifestsCount, availableAppliedObjectsCount, untrackableAppliedObjectsCount) |
There was a problem hiding this comment.
Nit: what if the appliedCondition (say false) does not agree with appliedManifestsCount (say == manifestCount)? I think it's slightly better to use the output of the setWorkAppliedCondition to determine the appliedCondition than using "appliedManifestsCount" as they are two source of truth
Comment on lines
+275
to
+281
| if appliedCond != nil { | ||
| meta.SetStatusCondition(&manifestCond.Conditions, *appliedCond) | ||
| } else { | ||
| // As the conditions are ported back; removal must be performed if the DiffReported | ||
| // condition is not set. | ||
| meta.RemoveStatusCondition(&manifestCond.Conditions, fleetv1beta1.WorkConditionTypeApplied) | ||
| } |
There was a problem hiding this comment.
I don't see any test changes that is related to most of the changes in this file
Comment on lines
+451
to
+452
| case appliedManifestCount != manifestCount: | ||
| // Not all manifests have been applied; skip updating the Available condition. |
There was a problem hiding this comment.
same here, no test change needed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of your changes
This PR tweaks how the new work applier sets conditions for the Work objects and their manifests.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer