-
Notifications
You must be signed in to change notification settings - Fork 260
detect template type from input, but respect explicit specification #1844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
detect template type from input, but respect explicit specification #1844
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1844 +/- ##
========================================
Coverage 57.50% 57.51%
========================================
Files 136 137 +1
Lines 12934 13035 +101
========================================
+ Hits 7438 7497 +59
- Misses 4341 4382 +41
- Partials 1155 1156 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Templateinterface 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.
32017ae to
23b2348
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7e6e569 to
0ff64da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
745a3f7 to
cd4bc48
Compare
alpha/template/template.go
Outdated
| // 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could returned reader be hidden inside the returned Template instance? can this reader be used independently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cd4bc48 to
9e351fc
Compare
|
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. |
alpha/template/basic/basic.go
Outdated
|
|
||
| type Template struct { | ||
| RenderBundle func(context.Context, string) (*declcfg.DeclarativeConfig, error) | ||
| func init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd initially tried to do that-- feeling similarly-- but I was also trying to optimize the smallest number of new packages and the resulting import cycles basically dictated this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another swing at this now that I had some time in the new year! 😄
| return schema | ||
| } | ||
|
|
||
| type BasicTemplateData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used by the template converter to be able to make a basic template from a full FBC representation, so it does need to be public.
I'm sure that I could pivot this away, but the purpose of this PR was to change the functional flow of the CLI commands to be able to simplify them (not needing to always specify the template type) and I could follow up with such things in later efforts.
| Bundles []semverTemplateBundleEntry `json:"bundles,omitempty"` | ||
| } | ||
|
|
||
| type SemverTemplateData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When initially implemented, this had to be public/exported in order for marshaling/unmarshaling to function. I haven't revisited.
6243e92 to
0b3a0cb
Compare
0b3a0cb to
ff10956
Compare
|
/lgtm |
Signed-off-by: grokspawn <jordan@nimblewidget.com>
Signed-off-by: grokspawn <jordan@nimblewidget.com>
Signed-off-by: grokspawn <jordan@nimblewidget.com>
ff10956 to
a4b0045
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grokspawn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
acfd70b
into
operator-framework:master
|
|
||
| func (r *registry) CreateTemplateByType(templateType string, renderBundle BundleRenderer) (Template, error) { | ||
| r.mu.RLock() | ||
| factory, exists := r.factories[templateType] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this change introduces a regression in v1.62.0 because the factories are indexed by the full schema but templateType is only the last portion.
Issue opened:
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
/docs