:seedling: Refactor MakeCSV utility to builder pattern by perdasilva · Pull Request #2244 · operator-framework/operator-controller

:seedling: Refactor MakeCSV utility to builder pattern by perdasilva · Pull Request #2244 · operator-framework/operator-controller

Description

Since we moved the bundlefs test utility to the builder pattern, this PR moves the MakeCSV test utility to the builder pattern for consistency.

Reviewer Checklist

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

Codecov Report

❌ Patch coverage is 80.43478% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.65%. Comparing base (fbd2e65) to head (9a18ee1).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...kpak/util/testing/clusterserviceversion/builder.go	80.43%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2244      +/-   ##
==========================================
+ Coverage   72.54%   72.65%   +0.10%     
==========================================
  Files          86       87       +1     
  Lines        8607     8593      -14     
==========================================
- Hits         6244     6243       -1     
+ Misses       1952     1940      -12     
+ Partials      411      410       -1

Flag	Coverage Δ
e2e	`38.70% <ø> (ø)`
experimental-e2e	`45.68% <ø> (+0.16%)`	⬆️
unit	`57.80% <80.43%> (-0.04%)`	⬇️

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

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

		b := source.FromBundle(
		bundle.RegistryV1{
		CSV: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)),
		CSV: csvbuilder.New().WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces).Build(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make it consistent with bundlefs.Builder() calls, how about that we also do csv.Builder() instance of csvbuilder.New()?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was my original approach but csv is such a common variable name I didn't want to overload it with the package. Wdyt of csvbuilder.Builder()? That was my first alternative, but then it felt weird to have csvbuilder.Builder() - but maybe that's ok?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, previously MakeCSV sounds also very generic/common, so I would go simply with csv.Builder() - adding redundant builder suffix does not bring much for clarity. Maybe to call the package clusterserviceversion and then we have clusterserviceversion.Builder()?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels so long though T_T

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that CSV is a particularity of this project. But, within this context, its widely used.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that CSV is a particularity of this project. But, within this context, its widely used.

If so, then csv.Builder() is pretty clear then.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just gone for the original suggestion. I don't like csv because it's a common variable name for the csv.
csv := csv.Builder().Build() feels weird to me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're just talking about some small test utility anyway.

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak, tmshort

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~internal/operator-controller/OWNERS~~ [tmshort]

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

openshift-ci bot added the approved

Indicates a PR has been approved by an approver from all required OWNERS files.

label