Optionally, overlay/insert via function

[mamachanko]

The insert overlay action has an optional 'via' kwarg, which receives the left and right node and produces the new right node, similar to the replace overlay action.

For example, this allows to add a ConfigMap into every matched Kubernetes Namespace without knowing their names in advance:

#@ load("@ytt:overlay", "overlay")

#@ for i in range(3):
---
apiVersion: v1
kind: Namespace
metadata:
  name: #@ i
#@ end

#@ def cm(ns):
apiVersion: v1
kind: ConfigMap
metadata:
  name: insert
  namespace: #@ ns["metadata"]["name"]
#@ end

#@overlay/match by=overlay.subset({"kind": "Namespace"}), expects="0+"
#@overlay/insert after=True, via=lamda ns,_: cm(ns)
---

produces:

apiVersion: v1
kind: Namespace
metadata:
  name: 0
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: insert
  namespace: 0
---
apiVersion: v1
kind: Namespace
metadata:
  name: 1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: insert
  namespace: 1
---
apiVersion: v1
kind: Namespace
metadata:
  name: 2
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: insert
  namespace: 2

---

Addresses #738

Qs for the maintainers:

• What do you think about the suggested API?
• @jtigger mentioned that: "the solution to this is likely to also resolve Panics when appending to "empty" #718". However, I don't see how.
• InsertAnnotation.Value is mostly a reproduction of ReplaceAnnotation.Value sans comments and todos. It felt not right to copy over those unless that is preferred.
• If via=lambda ... expects more than one arg it does not produce a great error message, much like overlay/replace's via. For example: overlay.apply: Document on line stdin:16: function lambda missing 2 arguments (... Would you consider improving that part of this PR?

Companion PR for docs: https://github.com/vmware-tanzu/carvel/pull/553

[pivotaljohn]

What do you think about the suggested API?

🤔 Any reason not to include the "right" value as an argument to via=?

It doesn't always come up, but doing so allows the author to highlight a YAML fragment as an important piece. e.g. resource limits that override a targeted PodSpec of some kind where the point is to replace them.

Doing so also makes it easier to re-use a function written for @overlay/replace with @overlay/insert.

Finally, it is consistent; one less thing to explain to authors.

Thoughts?

---

@jtigger #738 (comment) that: "the solution to this is likely to also resolve #718". However, I don't see how.

👎🏻 I was wrong about that. I assumed that since we were talking about inserting into an empty collection, that would have been part of the implementation, here. In fact, @overlay/append is not merely an @overlay/insert after=True. 🤔💭

---

InsertAnnotation.Value is mostly a reproduction of ReplaceAnnotation.Value sans comments and todos. It felt not right to copy over those unless that is preferred.

👍🏻 Yeah, the comments there don't seem to restore much (if any) context that the reader would already have.

---

If via=lambda ... expects more than one arg it does not produce a great error message, much like overlay/replace's via. For example: overlay.apply: Document on line stdin:16: function lambda missing 2 arguments (... Would you consider improving that part of this PR?

👌🏻 I wouldn't consider it to be required scope for this PR. @overlay/replace via= has behaved this way for years.

[pivotaljohn]

(Commentary, not a request for a change, per se.)

It's interesting that in @overlay/replace (and therefore here) we store the via= as a starlark.Value (rather than a starlark.Callable).
We also don't vet the argument, here — a natural choke-point for invalid input.

The advantage of vetting/constraining as early as possible is that it generally simplifies subsequent logic, reducing complexity overall.

Since there's very little distance between NewAssertionAnnotation() and Value() (below), I don't see that complexity reduction: just moving code around.

It's more meaningful as aiming for exemplary code (i.e. worthy of imitation).

It's not the responsibility of the author of this PR to "fix" this; nor is this a blocker for this PR (that's impractical).

[mamachanko]

I have refactored both annotations to store their via kwarg as a starklark.Callable. To avoid regression, the error case for both are covered with a template test.

My first intuition was to extend pkg.template.core.StarlarkValue with a AsCallable, but that does not work because Go has no function type that is arg-agnostic, which is why we like Go. And so, I settled for a straight-forward type assertion within the New* constructor of each annotation.

[pivotaljohn]

Companion PR for docs: https://github.com/vmware-tanzu/carvel/pull/553

Wow! This is wonderful! 🙏🏻

[mamachanko]

@pivotaljohn As per your very reasonable suggestion, the via kwarg now receives both the left and the right node. 🙇🏻‍♂️

[pivotaljohn]

Hey @mamachanko, thanks for your patience as we cleared the decks with the v0.43.0 release. 🙇🏻‍♂️

I walked through the flow again; this all looks spot on.
You included some tidying up and we appreciate that as well.
Thank you for the added tests.

[mamachanko]

@pivotaljohn wonderful! 🥹

You closed #718, but is it possible you meant to close #738 instead?

[pivotaljohn]

Oh wow... I think it was this comment that triggered the workflow: #738 (comment)

I'll be choosing my words more wisely, going forward.