ForEach reconciler #547
ForEach reconciler #547
Conversation
The ForEach reconciler can iterator over a slice calling a nested reconciler for each item. The current item is stashed on the context to be retrieved with a CursorStasher. Nested iterators are allowed so long as the type of the items being iterated over have unique names. We may enhance the generated stash key in the future to relax this requirement, so users should not assume the specific key to be stable between releases. Signed-off-by: Scott Andrews <scott@andrews.me>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #547 +/- ##
==========================================
+ Coverage 60.52% 60.60% +0.08%
==========================================
Files 32 32
Lines 2880 2924 +44
==========================================
+ Hits 1743 1772 +29
- Misses 1043 1057 +14
- Partials 94 95 +1 ☔ View full report in Codecov by Sentry. |
README.md
Outdated
|
|
||
| A [`ForEach`](https://pkg.go.dev/reconciler.io/runtime/reconcilers#ForEach) calls the reconciler for each item returned from Items. The current cursor can be retrieved with [`CursorStasher`](https://pkg.go.dev/reconciler.io/runtime/reconcilers#CursorStasher). | ||
|
|
||
| Nested iteration is allowed so long as the types being iterated over contain unique names. Otherwise the stash keys will collide. For testing the nested reconciler outside the scope of the loop, use the stasher's Key method to lookup the StashKey, do not expect the StashKey to be stable between releases. |
There was a problem hiding this comment.
[suggestion, non-blocking]: When reading "so long as the types being iterated over contain unique names" I was assuming that the items need to have unique names but it's speaking about types. I struggle to parse what it means for a "type" to "contain a name". The doc string of ForEach is a little clearer:
// Multiple ForEach reconcilers are nestable so long as the types being iterated over are unique.So the types must be unique then, I think. That makes sense, because we identify each nested reconciler by type.
Maybe this is better:
Nested iteration is allowed so long as each reconciler's type is unique.
There was a problem hiding this comment.
Types have names in go. For example, the type corev1.Pod has the name "Pod". Because the stash key is based on a string, we need to coerce the type to a string. In general this will be transparent to users, but if they have nested iteration of the same type or different types that have the same name, then a collision will occur.
I'll take a point to try to clean this up sooner rather than later.
| d.ContainerDie("die", func(d *diecorev1.ContainerDie) { | ||
| d.Image("die") |
There was a problem hiding this comment.
[issue, absolutely non-blocking]: overloading the term "die" with "exit early" is fine, but makes the reader squint
There was a problem hiding this comment.
the other meaning of "die" 😄
| SpecDie(func(d *dies.TestResourceSpecDie) { | ||
| d.Fields(map[string]string{}) | ||
| }) | ||
|
|
There was a problem hiding this comment.
[thought, off-topic]: In terms of reading order this is the moment where I expect to see the reconciler-under-test. I keep scrolling back and forth between the test cases and the reconciler below. It's absolutely fine. Declaring the controller here would work, but it's not something we do conventionally in our test suite. All good 😄
There was a problem hiding this comment.
there are places were we do define a unique reconciler for each test case. Since there are only a couple test cases I think this is cleaner, but if those cases needed to diverge more then it would be worth localizing the reconciler definition.
| @@ -174,7 +174,6 @@ func typeName(i interface{}) string { | |||
| } | |||
|
|
|||
| t := reflect.TypeOf(i) | |||
| // TODO do we need this? | |||
Co-authored-by: Max Brauer <mamachanko@users.noreply.github.com> Signed-off-by: Scott Andrews <scott@andrews.me>
The ForEach reconciler can iterator over a slice calling a nested reconciler for each item. The current item is stashed on the context to be retrieved with a CursorStasher.
Nested iterators are allowed so long as the type of the items being iterated over have unique names. We may enhance the generated stash key in the future to relax this requirement, so users should not assume the specific key to be stable between releases.