feat: drift detection and takeover implementation (1/)#985
feat: drift detection and takeover implementation (1/)#985michaelawyu merged 6 commits intoAzure:mainfrom
Conversation
| if ownerRefs == nil { | ||
| ownerRefs = []metav1.OwnerReference{} | ||
| } |
There was a problem hiding this comment.
I don't think you need this, the append func handles ownerRef == nil case
|
|
||
| // prepareExistingManifestCondQIdx returns a map that allows quicker look up of a manifest | ||
| // condition given a work resource identifier. | ||
| func prepareExistingManifestCondQIdx(existingManifestConditions []fleetv1beta1.ManifestCondition) map[string]int { |
There was a problem hiding this comment.
this function is very similar to the prepareRebuiltManifestCondQIdx function in status.go. Is there a way to unify them and move it to util.go?
There was a problem hiding this comment.
Hi Ryan! Yeah, the two functions are very similar; but currently I fear that I do not seem to have an elegant enough solution to unify the two.
I could use generics and write something like this:
// prepareManifestCondQIdx returns a map that allows quicker look up of a manifest
// condition given a work resource identifier.
func prepareManifestCondQIdx[E fleetv1beta1.ManifestCondition | *manifestProcessingBundle](S []E) map[string]int {
manifestQIdx := make(map[string]int)
for idx := range S {
E := S[idx]
var id *fleetv1beta1.WorkResourceIdentifier
mc, mcOK := any(E).(fleetv1beta1.ManifestCondition)
b, bOK := any(E).(*manifestProcessingBundle)
switch {
case mcOK:
id = &mc.Identifier
case bOK:
id = b.id
default:
// This will never run, or the code will not compile.
panic("unexpected type is passed in")
}
wriStr, err := formatWRIString(id)
if err != nil {
continue
}
manifestQIdx[wriStr] = idx
}
return manifestQIdx
}
But it seems to be overcomplicating things 😣 Alternatives include a) introducing an interface that allows retrieval of identifiers from ManifestCondition and manifestProcessingBundle types, or b) extracting the identifiers into a separate slice first, both might not be fully satisfactory (overkilling, or too much overhead) 😣 If you have a preference, please let me know.
|
Per offline discussions, will merge this PR now and open a new one to address comments. If there’s any further concern, please let me know. |
2 participants
Description of your changes
This PR adds the implementation of the new work applier with drift detection and takeover experience.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
As the new drift detection and takeover experience requires significant changes to the work applier (e.g., originally the work applier simply applies with no need to account for drifts/diffs/ownership mgmt, etc.), to avoid difficulties caused by maintenance of working intermediate states, the new experience will be submitted as a new controller that is not currently enabled.
A preview of the full experience is available in the preview PR (and in the preview env.).