Refactor: templates

[matejvasek]

Make fn.Template an interface instead of a concrete struct.

[codecov]

Codecov Report

Merging #961 (07c0aa4) into main (739cded) will increase coverage by 2.21%.
The diff coverage is 53.76%.

@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
+ Coverage   43.85%   46.06%   +2.21%     
==========================================
  Files          55       57       +2     
  Lines        5163     7454    +2291     
==========================================
+ Hits         2264     3434    +1170     
- Misses       2576     3695    +1119     
- Partials      323      325       +2     

Impacted Files	Coverage Δ
cmd/completion_util.go	0.00% <0.00%> (ø)
cmd/invoke.go	34.81% <0.00%> (-0.97%)	⬇️
docker/docker_client.go	82.52% <ø> (+0.57%)	⬆️
k8s/logs.go	0.00% <0.00%> (ø)
k8s/secrets.go	13.54% <0.00%> (-0.75%)	⬇️
s2i/builder.go	32.69% <32.69%> (ø)
invoke.go	52.99% <61.53%> (+2.99%)	⬆️
filesystem.go	64.96% <64.06%> (+5.46%)	⬆️
client.go	61.94% <66.66%> (+2.01%)	⬆️
cmd/build.go	60.49% <70.83%> (+0.86%)	⬆️
... and 58 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc3a418...07c0aa4. Read the comment docs.

[matejvasek]

@zroubalik @lkingland @salaboy PTAL

[lkingland]

I really do like this update very much. I hope my long comments below do not obscure that fact. The improved clarity of copying and ability to use different implementations of a Template is really going to be nice to have (especially Dynamic Templates!)

My suggestions boil down to:

• Pass data structures by value (unless sharing a memory location is actually needed)
• (related) Use non-pointer receivers for methods (unless sharing a memory location is actually needed)
• (related) Use non-pointer arguments and return new data structures with mutations applied rather than ask a method to go mutate a memory location
• Rely on Template.Write to decide whether or not to mask manifest.yaml, rather than masking it from the filesystem.
• Defaults should be applied in constructors (where used), with everywhere else failing fast
• Tests should use the API where possible and avoid hard-coding a specific cases or relying on a side-effect (where possible)
• Directly serialize Repository, Runtime and Template out at different levels rathern than using a shared nested config (high field overlap, but they are not the same!).
• Comments are a pain, but should really be expected, and follow the Go style of "all exported members have at least a sentence, beginning with the exported token".

/lgtm
/hold for others and for potentially update which address these above gentle suggestions

Overall looks good!

[lkingland]

Here also, to receive a Function as a reference is not really necessary since it is a data structure. Generally preferable to omit the pointer and pass by value unless its methods are clearly intended to mutate its internal state at a shared memory location.

As an example, here, simply calling .Write and passing it a *Function causes the passed function to have its state mutated. That is a side-effect and generally an antipattern unless the visitor pattern is clearly expected and required.

In the case where this really is what's desired, it even then is often better for a method to accept an immutable struct and return a new struct with modifications applied.

This is, for example, the logic behind the API of the built-in append. We do not:

append(&elements, "B")

Nor do we:

elements.append("B")

Rather, we:

elements = append(elements, "B")

I elaborate on the pros/cons in the other comment...

[matejvasek]

@lkingland Little bit out of topic: To be honest I have mixed feelings from append.
It doesn't modify slice you are appending to which is good.
However returned slice may or may not share underlying memory.

	s1 := make([]int, 1, 1024)
	s2 := append(s1, 1)

	fmt.Println(s1)
	s2[0] = 42 // it might look like you are not modifying s1 but you might be
	fmt.Println(s1)

It's not working like conj in Clojure where collections are immutable.

[lkingland]

I think the concept of a templateConfig might be complicating what could really just be one object.

If the reason for separating these fields out is such that their fields can be embedded in Repository and Runtime structs, then I have a whole other long-winded reason why that may not be necessary (tl;dr they share fields, but are not the same because there are indeed small diferences at each level).

This might actualy be simpler and easer kept as a single template struct, with a few duplicated fields as necessary.

[matejvasek]

then I have a whole other long-winded reason why that may not be necessary (tl;dr they share fields, but are not the same because there are indeed small diferences at each level).

@lkingland what are those differences?

[matejvasek]

To me it seems that BuildConfig, HealthEndpoints, BuildEnvs and Invocation are just defaults for fn.Function that can be defined at level of repository runtime, or template.
Is there any objective problem with that?

[matejvasek]

If the reason for separating these fields out is such that their fields can be embedded in Repository and Runtime structs,

That's exactly what I want to do in subsequent PR.

[lkingland]

If you do take my suggestion and keep template only, by combining bacl in templateConfig, you would just need to capitalize those fields you want serialized to disk (BuildConfig, Invocation)

[matejvasek]

see comment

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

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:

• OWNERS [lkingland,matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[matejvasek]

Pass data structures by value

@lkingland To be clear I am huge fan of referential transparency and immutability whenever possible.

But at least for Write() function if feels phony to pretend it's pure function. It definitely is not pure function.
It already has side-effects -- it writes to filesystem, plus it's named "Write" clearly indicating it mutates things.

If we want to play game of functional programming then we perhaps want to change it to something like:

func (t template) GenerateProject(fn Function) (outFn Function, projectFiles fs.FS, err error) {}

And let caller to do the write to filesystem.

[matejvasek]

@lkingland I addressed some of your comments:
diff: 99df905...07c0aa4

[matejvasek]

@lkingland please re-LGMT

[lkingland]

/lgtm

[matejvasek]

@zroubalik @salaboy if you don't have objections I'll merge this.

[matejvasek]

/unhold