Use sets.NewString from apimachinery

[mattmoor]

Changes

This replaces the use of map[string]struct{} with sets.String from K8s apimachinery.

This is a nice higher-level API to replace the Go idiom, which (while clever) isn't terribly approachable to folks unfamiliar with Go.

N/A internal cleanup

/kind cleanup
/assign @vdemeester @afrittoli @imjasonh

[tekton-robot]

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/workingdir_init.go	94.4%	92.9%	-1.6
pkg/reconciler/pipeline/dag/dag.go	98.9%	98.8%	-0.1

[imjasonh]

/lgtm
/approve

Thanks! 😎

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH

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 [ImJasonH]

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

[mattmoor]

/retest

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/workingdir_init.go	94.4%	92.9%	-1.6
pkg/reconciler/pipeline/dag/dag.go	98.9%	98.8%	-0.1

[bobcatfish]

Honestly this doesn't seem worth it to me. Going this route and being consistent would mean we are now gonna need to be vigilant and/or add automation to see if ppl us the go based approach, and it seems like the only thing we maybe gain is ppl not needing to learn the encountered[r.Name] = struct{}{} syntax which is really kind of a one time expense.

f _, ok := encountered[r.Name]; ok makes more sense to me than if encountered.Has(r.Name)

I don't want to die on this hill tho!

[bobcatfish]

My last 2 cents that I can't stop myself from expressing: there is something to be said for using a lang idiomatically. This feels like we are trying to make Go more similar to some other language that is more familiar to ppl, and I'd prefer we use Go idioms instead.

[ghost]

@bobcatfish I tend to agree with you. And in hindsight I think this perspective would have similarly resulted in me not creating #2587. Is there something we can do to codify this approach in our docs?

[mattmoor]

@bobcatfish Ack on most of these points, but I'll add a bit of my own color to this as well. Sorry this turned into a wall of text. 😬

Regarding the "one time cost", while I agree that there is a one-time hit to learn the idiom (vs. say map[string]bool), I disagree that the resulting states require an equivalent level of cognitive load to understand. This isn't about making this look like another language, even when folks have been doing this for years (🙋 ), you have to effectively decode the idiom as you are reading, and (for me) that is strictly more expensive than simply reading a clearly named type or method (I'm reminded of literate programming). So I don't agree that "the only thing we gain" is folks not needing to learn the idiom.

This becomes especially true when the sets become elements of a larger type, e.g. func (string, map[string]map[string]struct{}) map[string]struct{} vs. func(string, map[string]sets.String) sets.String (there were a number of rewrites like this in this PR).

Furthermore, there are gains beyond simple readability from this as well, such as having methods for walking the deduplicated ordered members:
https://github.com/tektoncd/pipeline/pull/2909/files#diff-751973423f2da1201a11e2c0e41930ccR48

... or should the need arise performing other "set" operations like difference and intersection.

Many Go idioms like map[string]struct{} vs. map[string]bool and chan struct{} vs. chan bool arise because of their inherently superior efficiency over the other approach. With zero-values if myset[key] { is strictly more readable than if _, ok: myset[key]; ok {, but the latter with struct{} is more efficient. Encapsulation enables us to have this efficiency without sacrificing readability.

While this library may be overkill for many of the cases rewritten, I do think that it almost universally improves the readability without any material runtime cost (it is after all a type alias of the Go idiom), and I think there are instances (as above) where it eliminates one-off code. Good use of Go idioms and good use of libraries shouldn't be at odds with each other, and while I value consistency I think this is as much about educating contributors and reviewers about available libraries as it is Go idioms. I think that understanding of available libraries better prepares both sets of folks (no pun intended) to exercise good judgement about what's best for their particular situation.

I too don't plan to die on this hill 😅 , so if you don't want this PR, I would be happy to roll it back, but it sounded like @imjasonh and @afrittoli thought this was a positive change (here and in #pipelines-dev) 🤷

[bobcatfish]

@bobcatfish Ack on most of these points, but I'll add a bit of my own color to this as well. Sorry this turned into a wall of text. 😬

np np! my apologies for not replying faster also

you have to effectively decode the idiom as you are reading, and (for me) that is strictly more expensive than simply reading a clearly named type or method (I'm reminded of literate programming).

I agree. It makes me wonder: why did Go make this the idiom? If a language's idioms are too hard to understand, to me that points toward a problem with the language.

Maybe we decide: we are using Go because of XYZ (I'm guessing concurrency support) however we think its idioms are inscrutable, therefore we are hiding them. I'm probably being unreasonable thinking that it's that simple, but in general what I want to do is embrace the language I'm using. If there are problems with the language, it feels like the place to deal with that is in the language itself. (maybe that's happening already...)

func (string, map[string]map[string]struct{}) map[string]struct{} vs. func(string, map[string]sets.String) sets.String

That's a good point, I hadn't thought that through.

Maybe once Go has generics this will all be a moot point - until then it seems like the decision was made that it was okay for folks to duplicate this kinda code when they need it:

// Empty is public since it is used by some internal API objects for conversions between external
// string arrays and internal sets, and conversion logic requires public types today.
type Empty struct{}

// sets.String is a set of strings, implemented via map[string]struct{} for minimal memory consumption.
type String map[string]Empty

Kinda like how it's okay to have to go if err all over the place. I think they might be walking back some of these decisions tho?

Maybe what really bothers me is that this code is inside of k8s api machinery, i.e. we are depending on a lib inside of k8s - fortunately we already rely on it, so maybe that makes sense. I don't know if I'd be having the same reaction if someone had written a set package in this codebase.

Maybe you're right about the other functionality we get - and maybe once Go has generics this will all be moot and the Go standard libs will include more of this kind of thing.

if you don't want this PR, I would be happy to roll it back,

nah that's okay - thanks for taking the time to discuss this!

[mattmoor]

It makes me wonder: why did Go make this the idiom?

Feels like backfill to me. "Oh shoot, people want sets!" "What is the most efficient way to do this within the language?"

pissing contest ensues

Profit!