search

vmware-labs / reconciler-runtime

⚠️ Maintenance suspended. Please, migrate to the active fork reconciler.io/runtime. See https://github.com/reconcilerio/runtime/releases/tag/v0.20.0 for instructions. This repository will be archived eventually.
Other
81 stars 18 forks source link

List children exactly once for ChildSetReconciler #494

Closed scothis closed 8 months ago

scothis commented 8 months ago

Previously, the ChildSetReconciler would list children once for the set and again for each child being reconciled. At best this was inefficient, at worst it introduced subtile issues when the content of the listing changed over the course of the reconcile.

Now the content of the list from the ChildSetReconciler is reused by each child that is reconciled.

Resolves #481

scothis commented 8 months ago

cc @squeedee

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 61.13%. Comparing base (e9224d1) to head (e90766a).

Files Patch % Lines
testing/client.go 0.00% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #494 +/- ## ========================================== - Coverage 61.18% 61.13% -0.06% ========================================== Files 28 28 Lines 2566 2583 +17 ========================================== + Hits 1570 1579 +9 - Misses 909 917 +8 Partials 87 87 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

squeedee commented 8 months ago

I really like the calledAtMostTimes solution for the test.

squeedee commented 8 months ago

I went back to the state of my project that caused me to raise this issue, passing 2/10 times, then pointed at this branch and reran, got 10/10 passes, so I have a lot of confidence this resolves the issue. Thanks!

vmwclabot commented 7 months ago

@scothis, VMware has approved your signed contributor license agreement.