Part 1: Reduce number of variable sources. Bundle uniqueness

[m1kola]

Description

Spliting #460 into smaller chunks. Related to #437

In this part I extract code related to bundle uniqueness from CRDUniquenessConstraintsVariableSource into a separate function.

CRDUniquenessConstraintsVariableSource will be removed later in #501

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[stevekuznetsov]

If you do not have sub-tests, you don't need (and should not use) t.Run()

[m1kola]

I think I'll keep this subtest. Eventually we will need uniqueness to be applied also based on CRDs. I would imagine it will be a separete subtest.

I believe it doesn't hurt.

[stevekuznetsov]

My point was that it's not a "sub" test since it's the only one. When and if the future comes where you need them, add them by all means!

[ncdc]

t.Run does not follow the same philosophy as Ginkgo's Describe/It blocks, FYI. We really only use it for table-style tests to denote which test case we're running.

[m1kola]

@ncdc @stevekuznetsov I'm not convinced. t.Run has many uses. Yes, table-style tests is one of them, but you can also use it for creating hierarchical tests and separating test cases from setup/tear down code.

I like how it separates generic catalog data from test case data.

I think Go testing framework is not as prescriptive as you might think: I do not see any documentation that supports the point that I'm doing something ilegal here.

[m1kola]

I made the change requested because I want to make progress and I do not think this detail is important enough to spend more cycles on this.

However I do not want this to create a precedent for the future.

I would like us to collaborate instead of prescribing each other specific ways. Usually it is possible to reach some consensus based on technical facts.

We can also discuss and document specific decisions or even code them into tools. E.g. if we want subtests to be used only for table-style tests - either all of our tests must be consistent or it should be discussed with the community (like we did here when we decied to get rid of ginkgo) and ideally documented in some sort of style guide.

[ncdc]

Links 1 and 2 are not how we should be writing tests.

There is no reason for links 4 or 5 to use t.Run for a subtest.

When I review PRs, I have certain things I look for. This is one of them. Please do not use t.Run for unnecessary subtests.

[m1kola]

Links 1 and 2 are not how we should be writing tests.

There is no reason for links 4 or 5 to use t.Run for a subtest.

@ncdc Could you please explain why? I believe I demonstrated evidence that previous arguments are not valid. Is there a technical reason for that? Do we have a OF agreed document/style guide am not aware of?

Multiple people authored it and multiple people approved each of these change sets. So opinions on this differ.

When I review PRs, I have certain things I look for. This is one of them. Please do not use t.Run for unnecessary subtests.

I would like us to think collaboratively and avoid things like "please do it this way because I said so".

Maybe we should adopt something like this? See principles section:

• Technical facts and data overrule opinions and personal preferences.

• On matters of style, the style guide is the absolute authority. Any purely style point (whitespace, etc.) that is not in the style guide is a matter of personal preference. The style should be consistent with what is there. If there is no previous style, accept the author’s.

• Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.

• If no other rule applies, then the reviewer may ask the author to be consistent with what is in the current codebase, as long as that doesn’t worsen the overall code health of the system.

[ncdc]

I am happy to write it down. There is no precedent to use t.Run in this way. This is also an experiential thing... so sometimes "please do it this way because I said so" is backed by years of experience reading, writing, and reviewing code.

[m1kola]

I am happy to write it down.

@ncdc thanks. Google has very nice guidance for reviewers and authors as well as style guide for different languages. I think we can learn a lot from it and can probably borrow some stuff from it.

I personally prefer not to have formal documents like this but it looks like we won't be able to work productively if we continue discuss matters like this in each PR.

There is no precedent to use t.Run in this way.

But I provided evidence that it is used this way in our codebase and in golang/go codebase...

This is also an experiential thing... so sometimes "please do it this way because I said so" is backed by years of experience reading, writing, and reviewing code.

Please do not dismiss years of my experience and experience of other authors like that.

I think it is not a good idea to evaluate arguments on the accomplishments/success of the person making the argument. Let's evaluate on the merits of the argument itself.

It will be very hard to build a welcoming community with approach like this.

---

Back to the PR itself.

@ncdc @stevekuznetsov given that the PR was updated with the requested change - are we good to go? Or do you have any other feedback?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7411e68) 83.75% compared to head (2ac8ff8) 83.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   83.75%   83.92%   +0.16%     
==========================================
  Files          23       23              
  Lines         868      877       +9     
==========================================
+ Hits          727      736       +9     
  Misses         96       96              
  Partials       45       45              

Flag	Coverage Δ
e2e	65.45% <100.00%> (+0.35%)	⬆️
unit	78.68% <100.00%> (+0.23%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...al/resolution/variablesources/bundle_uniqueness.go	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[m1kola]

Rebased on top of main. Need this for rest of the PRs in the series. No other changes apart from the rebase since yesterday.

[joelanford]

One performance nit. It's unclear how much of a difference it would make.

(Note: it would be nice to have some resolver benchmarks that could give us insight on resolver performance and help us optimize and prevent regressions)

[m1kola]

@joelanford @stevekuznetsov I addressed the latest comments. Please take another look.

[m1kola]

Had to re-push due to github.com/google/go-cmp moving from inderect dependency to derect one (go.mod needed an update). No other changes apart from this.