🌱 Refactor MakeCSV utility to builder pattern #2244
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| b := source.FromBundle( | ||
| bundle.RegistryV1{ | ||
| CSV: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)), | ||
| CSV: csvbuilder.New().WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces).Build(), |
There was a problem hiding this comment.
to make it consistent with bundlefs.Builder() calls, how about that we also do csv.Builder() instance of csvbuilder.New()?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
feels so long though T_T
There was a problem hiding this comment.
I get that CSV is a particularity of this project. But, within this context, its widely used.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We're just talking about some small test utility anyway.
perdasilva
force-pushed
the
refactor-makecsv
branch
2 times, most recently
from
9277daa to
459fab3
Compare
Signed-off-by: Per Goncalves da Silva <[email protected]>
perdasilva
force-pushed
the
refactor-makecsv
branch
from
459fab3 to
9a18ee1
Compare
internal/operator-controller/rukpak/render/registryv1/registryv1_test.go
Show resolved
Hide resolved
internal/operator-controller/rukpak/render/registryv1/registryv1_test.go
Show resolved
Hide resolved
|
/approve |
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 35da385
into
operator-framework:main
3 participants
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