template/*: allow passing custom bundle renderer #1423
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford 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
|
Seems like the semver template explicitly expects the references in the template file to show up in the rendered FBC. Looking to see if that can be relaxed somehow because that breaks the use case I'm trying to make work (render a bundle from a local OCI archive file) |
joelanford
force-pushed
the
template-custom-renderer
branch
from
4693c71
to
a8d127c
Compare
joelanford
force-pushed
the
template-custom-renderer
branch
from
a8d127c
to
f262850
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1423 +/- ##
==========================================
+ Coverage 48.14% 48.30% +0.15%
==========================================
Files 136 136
Lines 12780 12798 +18
==========================================
+ Hits 6153 6182 +29
+ Misses 5593 5581 -12
- Partials 1034 1035 +1 ☔ View full report in Codecov by Sentry. |
| buildBundleList(&sv.Fast.Bundles, &bundleDict) | ||
| buildBundleList(&sv.Stable.Bundles, &bundleDict) | ||
|
|
||
| bundleDict := buildBundleList(*sv) |
There was a problem hiding this comment.
nit: would it be worth renaming sv to make the code a bit more readable?
| (*dict)[b.Image] = struct{}{} | ||
| func buildBundleList(t semverTemplate) map[string]string { | ||
| dict := make(map[string]string) | ||
| for _, bl := range []semverTemplateChannelBundles{t.Candidate, t.Fast, t.Stable} { |
There was a problem hiding this comment.
nit: wdyt about making the making the semverTemplateChannelBundles list a parameter? Before we could see which channel templates were being used to create the bundle list at the top level. Might also make the method more flexible? (though given that it isn't exported - maybe not...XD).
There was a problem hiding this comment.
If I'm understanding it right, aside from updating some error messages to be more readable (thank you!), we're moving the Template type(s) to be more flexible, allowing users to implement their own rendering behavior. In general it lgtm. I've left a few comments and suggestions, but nothing that I think is blocking or that we couldn't address later. I don't know enough about the downstream (not as in to the downstream) side-effects of this change to approve. I'd think @grokspawn would be the best person here to reason about the big picture effects of this change (though I doubt there will be anything too serious).
| "io" | ||
|
|
||
| "github.com/blang/semver/v4" | ||
|
|
||
| "github.com/operator-framework/operator-registry/alpha/action/migrations" | ||
| "github.com/operator-framework/operator-registry/pkg/image" | ||
| "github.com/operator-framework/operator-registry/alpha/declcfg" | ||
| ) | ||
|
|
||
| // data passed into this module externally | ||
| type Template struct { |
There was a problem hiding this comment.
suggestion: should we just make Template (as defined in basic with just the RenderBundle method) a shared type that basic and semver sub-type and implement? The basic and semver packages could even export a factory functions to get new instances of templates?
|
/lgtm |
openshift-merge-bot
bot
merged commit 1adde98
into
operator-framework:master
|
@joelanford Oh shit! ocp-bot merged it coz I put /lgtm 🫤 |
Description of the change:
Make the template rendering libraries accept a bundle renderer function.
Motivation for the change:
This allows importers of operator-registry to provide custom rendering logic for bundle references that appear in template configuration files.
This would enable third-parties to build experimental bundle transport formats and continue using the standard operator-registry template tooling with them.
Reviewer Checklist
/docs