detect template type from input, but respect explicit specification #1844

grokspawn · 2025-11-25T03:41:07Z

Description of the change:
Replaces #1814

Various type changes and refactoring necessary to make opm alpha render-template capable of detecting the template type from sniffing its schema, as well as name standardization across all template types and a new factory-based creation pattern.

This absolutely sacrifices stability of the API in favor of stability of the datatypes and callflows for future growth.

Motivation for the change:
Since we created a formal schema for the basic catalog template, it made much more sense for Template to be an interface for a creation/call pattern which was increasingly entrenched in users' tooling... so it was easy to put off. But since it was also largely a "move existing stuff around" activity (read: super simple operations with complex propagation and high potential for side effects), it seemed a great opportunity to see what Claude would do with it.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci · 2025-11-25T03:41:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2025-11-25T03:45:35Z

Codecov Report

❌ Patch coverage is 59.39850% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.57%. Comparing base (e7b9dee) to head (9e351fc).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
alpha/template/basic/basic.go	0.00%	29 Missing ⚠️
alpha/template/semver/semver.go	57.69%	11 Missing ⚠️
alpha/template/registry.go	18.18%	9 Missing ⚠️
alpha/template/template.go	94.00%	3 Missing ⚠️
alpha/template/schema.go	88.23%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
+ Coverage   57.52%   57.57%   +0.04%     
==========================================
  Files         136      138       +2     
  Lines       12934    13038     +104     
==========================================
+ Hits         7440     7506      +66     
- Misses       4339     4376      +37     
- Partials     1155     1156       +1

☔ 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.

Copilot

Pull request overview

This PR refactors the template rendering system to support automatic template type detection based on schema fields, while still allowing explicit type specification. The changes consolidate the previously separate basic and semver subcommands into a unified render-template command that can auto-detect the template type from the input file's schema field.

Key Changes:

Introduced a common Template interface and factory pattern for all template types
Implemented schema-based auto-detection using a registry system
Unified the command structure from separate subcommands to a single command with optional type specification

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`cmd/opm/alpha/template/cmd.go`	Replaced separate subcommands with unified `render-template` command that accepts optional TYPE and FILE arguments
`cmd/opm/alpha/template/render.go`	New file implementing unified rendering logic with auto-detection and explicit type specification support
`cmd/opm/alpha/template/basic.go`	Removed - functionality moved to unified render.go
`cmd/opm/alpha/template/semver.go`	Removed - functionality moved to unified render.go
`alpha/template/template.go`	New file defining Template interface, TemplateFactory interface, and Registry for schema-based template creation
`alpha/template/schema.go`	New file implementing schema detection from YAML/JSON input
`alpha/template/basic/basic.go`	Refactored to implement new Template interface with factory pattern; renamed data struct to BasicTemplateData
`alpha/template/semver/semver.go`	Refactored to implement new Template interface with factory pattern; moved type definitions from types.go and renamed to SemverTemplateData
`alpha/template/semver/types.go`	Removed - content migrated into semver.go
`alpha/template/semver/semver_test.go`	Updated test assertions to use SemverTemplateData and changed to ElementsMatch for order-independent comparison

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

cmd/opm/alpha/template/render.go

cmd/opm/alpha/template/cmd.go

cmd/opm/alpha/template/render.go

alpha/template/template.go

cmd/opm/alpha/template/render.go

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

cmd/opm/alpha/template/render.go

alpha/template/template.go

alpha/template/schema.go

cmd/opm/alpha/template/cmd.go

cmd/opm/alpha/template/render.go

alpha/template/semver/semver_test.go

cmd/opm/alpha/template/render.go

Signed-off-by: grokspawn <jordan@nimblewidget.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

cmd/opm/alpha/template/render.go

alpha/template/template.go

alpha/template/semver/semver.go

alpha/template/basic/basic.go

pedjak · 2025-12-03T13:41:36Z

alpha/template/basic/basic.go

+}

-	return bt, nil
+// RenderBundle implements the template.Template interface


these docs lines are not very much helpful, can you expand for clarity?

pedjak · 2025-12-03T13:42:38Z

alpha/template/template.go

+	"github.com/operator-framework/operator-registry/alpha/declcfg"
+)
+
+// BundleRenderer defines the function signature for rendering bundle images


what is the semantic of string arg, what does it contain?

can you expand on the doc as well?

pedjak · 2025-12-03T13:44:20Z

alpha/template/template.go

+// BundleRenderer defines the function signature for rendering bundle images
+type BundleRenderer func(context.Context, string) (*declcfg.DeclarativeConfig, error)
+
+// Template defines the common interface for all template types


please expand on what the template is?

pedjak · 2025-12-03T14:41:26Z

alpha/template/template.go

+
+// TemplateRegistry maintains a mapping of schema identifiers to template factories
+type TemplateRegistry struct {
+	factories map[string]TemplateFactory


how about to use set.Set type here?

We're mapping between the schema identifiers for templates supported by the factory, and the generator function stored in the factory, so a set would only make sense here if our type were self-expressing or we validated fitness characteristics before invoking it, whereas this approach allows us to invoke non-error returns.

alpha/template/template.go

pedjak · 2025-12-03T15:14:53Z

alpha/template/template.go

+// and returns a reader that can be used to render the template. The returned reader includes
+// both the data consumed during schema detection and the remaining unconsumed data.


could returned reader be hidden inside the returned Template instance? can this reader be used independently?

It could be hidden in the template type, but it's arbitrary. The template is only interested in the reader as an input source, but it isn't inherently a template concern. It's just a general I/O one.

alpha/template/template.go

Signed-off-by: grokspawn <jordan@nimblewidget.com>

grokspawn · 2025-12-11T21:19:32Z

Thanks for the really detailed review @pedjak! I've included changes which hopefully resolve your concerns and address ambiguity in the PR. Please give it another once over when you have a chance.

pedjak · 2025-12-15T13:19:41Z

alpha/template/registry.go


 // GetTemplateRegistry returns the global template registry
-func GetTemplateRegistry() *TemplateRegistry {
+func GetTemplateRegistry() *templateRegistry {


can we return here an interface instead?

pedjak · 2025-12-16T14:04:44Z

alpha/template/basic/basic.go


-type Template struct {
-	RenderBundle func(context.Context, string) (*declcfg.DeclarativeConfig, error)
+func init() {


this init funciton is called only when we import alpha/template/basic package. Hence, only then we are going to register this Factory.

IMHO, we should register it at the other end - within alpha/template/registry.go - in that way all available factories will be registered always.

pedjak · 2025-12-16T14:06:43Z

alpha/template/basic/basic.go

+type Factory struct{}
+
+// CreateTemplate implements the template.TemplateFactory interface
+func (f *Factory) CreateTemplate(renderBundle template.BundleRenderer) template.Template {


given that we call New() from Factory, should we actually have New() function public?

pedjak · 2025-12-16T14:12:31Z

alpha/template/basic/basic.go

+	return schema
+}
+
+type BasicTemplateData struct {


does it need to be public?

pedjak · 2025-12-16T14:13:58Z

alpha/template/semver/semver.go


-func (t Template) Render(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
+// IO structs -- BEGIN
+type semverTemplateBundleEntry struct {


given that we are in semver package, I would suggest to drop semver prefix from the types declared here, the redundancy does not bring much.

pedjak · 2025-12-16T14:16:00Z

alpha/template/semver/semver.go

+	Bundles []semverTemplateBundleEntry `json:"bundles,omitempty"`
+}
+
+type SemverTemplateData struct {


does this need to be public? It is a bit strange that a caller get access to a type, that has member fields of a private type.

pedjak · 2025-12-16T14:20:29Z

alpha/template/semver/semver.go

+type Factory struct{}
+
+// CreateTemplate implements the template.TemplateFactory interface
+func (f *Factory) CreateTemplate(renderBundle template.BundleRenderer) template.Template {


same question as for the basic factory.

pedjak · 2025-12-16T14:20:57Z

alpha/template/semver/semver.go

+const schema string = "olm.semver"
+
+func init() {
+	template.GetTemplateRegistry().Register(&Factory{})


same question as for basic factory.

pedjak · 2025-12-16T14:21:12Z

alpha/template/semver/semver.go

+	return schema
+}
+
+const schema string = "olm.semver"


can you move it at the top of the file?

pedjak · 2025-12-16T14:21:33Z

alpha/template/semver/semver.go

+type channelArchetype string
+
+const (
+	candidateChannelArchetype channelArchetype = "candidate"
+	fastChannelArchetype      channelArchetype = "fast"
+	stableChannelArchetype    channelArchetype = "stable"
+)
+
+// mapping channel name --> stability, where higher values indicate greater stability
+var channelPriorities = map[channelArchetype]int{candidateChannelArchetype: 0, fastChannelArchetype: 1, stableChannelArchetype: 2}


can you move it to the top of the file?

openshift-ci bot requested review from anik120 and ankitathomas November 25, 2025 03:41

grokspawn requested a review from Copilot November 25, 2025 03:41

Copilot started reviewing on behalf of grokspawn November 25, 2025 03:41 View session

Copilot finished reviewing on behalf of grokspawn November 25, 2025 03:45

Copilot AI reviewed Nov 25, 2025

View reviewed changes

grokspawn force-pushed the template-autodetect branch 2 times, most recently from 32017ae to 23b2348 Compare November 25, 2025 17:49

grokspawn requested a review from Copilot November 25, 2025 17:55

Copilot started reviewing on behalf of grokspawn November 25, 2025 17:55 View session

Copilot finished reviewing on behalf of grokspawn November 25, 2025 17:59

Copilot AI reviewed Nov 25, 2025

View reviewed changes

grokspawn force-pushed the template-autodetect branch from 23b2348 to 938b01d Compare November 25, 2025 21:37

detect template type from input, but respect explicit specification

11883a1

Signed-off-by: grokspawn <jordan@nimblewidget.com>

grokspawn force-pushed the template-autodetect branch 3 times, most recently from 7e6e569 to 0ff64da Compare November 25, 2025 22:14

grokspawn requested a review from Copilot November 26, 2025 03:05

Copilot started reviewing on behalf of grokspawn November 26, 2025 03:06 View session

Copilot finished reviewing on behalf of grokspawn November 26, 2025 03:09

Copilot AI reviewed Nov 26, 2025

View reviewed changes

grokspawn force-pushed the template-autodetect branch 2 times, most recently from 745a3f7 to cd4bc48 Compare November 26, 2025 03:49

grokspawn requested a review from perdasilva November 26, 2025 04:14

pedjak reviewed Dec 3, 2025

View reviewed changes

review mods

9e351fc

Signed-off-by: grokspawn <jordan@nimblewidget.com>

grokspawn force-pushed the template-autodetect branch from cd4bc48 to 9e351fc Compare December 11, 2025 21:06

pedjak reviewed Dec 15, 2025

View reviewed changes

pedjak reviewed Dec 16, 2025

View reviewed changes

		// and returns a reader that can be used to render the template. The returned reader includes
		// both the data consumed during schema detection and the remaining unconsumed data.

detect template type from input, but respect explicit specification #1844

Are you sure you want to change the base?

detect template type from input, but respect explicit specification #1844

Uh oh!

Conversation

grokspawn commented Nov 25, 2025

Uh oh!

openshift-ci bot commented Nov 25, 2025

Uh oh!

codecov bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grokspawn commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2025 •

edited

Loading