-
Notifications
You must be signed in to change notification settings - Fork 233
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
Only run update logic if objects are actually different #160
Only run update logic if objects are actually different #160
Conversation
/retest |
1 similar comment
/retest |
pkg/apply/apply.go
Outdated
if err := MergeObjectForUpdate(existing, obj); err != nil { | ||
return errors.Wrapf(err, "could not merge object %s with existing", objDesc) | ||
} | ||
creationTimeStamp := existing.GetCreationTimestamp() |
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.
I would prefer if this block of code was part of MergeObjectForUpdate
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.
so pushing down the comparison down to Merge?
Just moving the 'backup' fields logic won't do.
/retest |
One dockblock update, then this looks good. Just rebase + squash, and we can get this merged. |
8d1796c
to
32265d0
Compare
/retest |
/lgtm |
/retest |
2 similar comments
/retest |
/retest |
/retest |
3 similar comments
/retest |
/retest |
/retest |
CI seems to be pretty reliable, >75% pass rate. I'd dig in to the artifacts (cluster logs) and see if something's not working. |
/retest |
2 similar comments
/retest |
/retest |
/approve |
er, |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rcarrillocruz, squeed 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 |
Given that @danwinship's comments were addressed, |
oh, sorry, I missed that it was stuck on my hold |
Fixes SDN-387