feat: enable work generator to process apply strategy updates + DiffReported condition #1016
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
| @@ -700,15 +703,16 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee | |||
| } else { | |||
| // we already checked the label in fetchAllResourceSnapShots function so no need to check again | |||
| resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot) | |||
| if workResourceIndex == resourceIndex { | |||
| // no need to do anything if the work is generated from the same resource/override snapshots | |||
| if workResourceIndex == resourceIndex && equality.Semantic.DeepEqual(existingWork.Spec.ApplyStrategy, newWork.Spec.ApplyStrategy) { | |||
There was a problem hiding this comment.
should we also check applyStrategy in the areAllWorkSynced() function?
There was a problem hiding this comment.
Ah, this is actually a tricky one; to avoid apply strategy inconsistencies the refresh has to be handled before the check. I will some edits
Comment on lines
+1819
to
+1829
| It("can update work status", func() { | ||
| work := retrieveWork(memberClusterNamespaceName, fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName)) | ||
|
|
||
| diffReportedCond := &metav1.Condition{ | ||
| Type: string(placementv1beta1.WorkConditionTypeDiffReported), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(workapplier.ManifestProcessingReportDiffResultTypeNoDiffFound), | ||
| ObservedGeneration: work.GetGeneration(), | ||
| } | ||
| refreshWorkStatus(work, nil, nil, diffReportedCond, nil) | ||
| }) |
There was a problem hiding this comment.
we won't need this (which simulated a behavior of another controller) if we explicitly set the applied to be false ourself.
There was a problem hiding this comment.
Hi Ryan! Part of the code tests the status populator, which is why the condition is being set (summarizing all DiffReported conditions from work objects).
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:
a) fixes the issue where work generator will not refresh apply strategies; and
b) enables work generator to process
DiffReportedconditionI have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
E2E tests can only be added when the new work applier is enabled.