feat: enable work generator to process apply strategy updates + DiffReported condition#1016
feat: enable work generator to process apply strategy updates + DiffReported condition#1016michaelawyu merged 16 commits intoAzure:mainfrom
Conversation
| if err != nil { | ||
| return err | ||
| } | ||
| if updated { |
There was a problem hiding this comment.
this will revert the applied condition on the binding. It seems that it's fine but please think through.
There was a problem hiding this comment.
Hi Ryan! I've been given this some more thought; this would only happen when the apply strategy does change, which should justify the the reset of the Applied condition; for partial failures the reset also seems to make sense (i.e., works are not even synchronized).
There was a problem hiding this comment.
for example, if the applyStrategy flipped allow Co-own from true to false, would it trigger any change on the workApplier?
There was a problem hiding this comment.
Hi Ryan! It might be (if a custom owner has been added before).
|
Latest incorporated changes:
|
|
|
||
| driftedManifests := extractDriftedResourcePlacementsFromWork(w) | ||
| driftedResourcePlacements = append(driftedResourcePlacements, driftedManifests...) | ||
| default: |
There was a problem hiding this comment.
isn't it better to explicitly have a case for that and then add an unexpected error in the default case so that we know we didn't miss anything.
There was a problem hiding this comment.
Got it; will send a subsequent PR.
ryanzhang-oss
left a comment
ryanzhang-oss
left a comment
There was a problem hiding this comment.
Please address the comments on a follow up PR
2 participants
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.