✨Add DeploymentConfig support to registry+v1 bundle renderer #2469
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| // DeploymentConfig is a type alias for v1alpha1.SubscriptionConfig | ||
| // to maintain clear naming in the OLMv1 context while reusing the v0 type. | ||
| type DeploymentConfig = v1alpha1.SubscriptionConfig |
There was a problem hiding this comment.
I thought we weren't going to use SubscriptionConfig because of the extra selector field?
There was a problem hiding this comment.
From the RFC:
NOTE: The v0 SubscriptionConfig struct also has a [Selector](https://github.com/operator-framework/api/blob/master/pkg/operators/v1alpha1/subscription_types.go#L47) field, however it is completely ignored by the v0 controller. According to the comment in the code, it was meant to:
Select which pods/ReplicaSets would receive the subscription’s configuration
Match pod template labels(similar to Deployment selectors)
However, the code that extracts subscription config overrides(pkg/controller/operators/olm/overrides/config.go) extracts all other fields like NodeSelector, Tolerations, Resources and are subsequently applied, but the Selector field is never extracted.
OLMv1 will maintain the same behavior—accepting but ignoring this field.
Notice that there's no applySelectorConfig() function, so we're just ignoring that field.
With our wish to reuse v0 SubscriptionConfig for maintainability, we carry over nuance/s into v1. Silently ignoring the selector field being one of them.
We have to remember to document this explicitly for our users everywhere for added clarity.
anik120
requested review from
perdasilva and
tmshort
and removed request for
Copilot and
oceanc80
This PR implements **Phase 2** of the [Deployment Configuration RFC](https://docs.google.com/document/d/18O4qBvu5I4WIJgo5KU1opyUKcrfgk64xsI3tyXxmVEU/edit?tab=t.0): extending the OLMv1 bundle renderer to apply `DeploymentConfig` customizations to operator deployments. Building on the foundation from operator-framework#2454, this PR enables the renderer to accept and apply deployment configuration when rendering registry+v1 bundles. The implementation follows OLMv0's behavior patterns to ensure compatibility and correctness. The next PR will wire up the config in the `ClusterExtension` controller by parsing `spec.install.config` to convert to `DeploymentConfig` and thread `DeploymentConfig` through the controller's render call chain
anik120
force-pushed
the
config-api-update-renderer
branch
from
07bd38a to
f8d0a5a
Compare
There was a problem hiding this comment.
Pull request overview
Implements Phase 2 of the Deployment Configuration RFC for registry+v1 bundles by threading a DeploymentConfig through the renderer and applying those customizations to CSV-generated Deployment resources (matching OLMv0 behavior), with extensive unit test coverage.
Changes:
- Add
DeploymentConfigsupport torender.Optionsand a newrender.WithDeploymentConfig(...)option. - Apply deployment customizations (env/envFrom, volumes, mounts, tolerations, resources, node selector, affinity, annotations) during registry+v1 CSV
Deploymentgeneration. - Add/extend unit tests covering option propagation and deployment customization behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/rukpak/render/render.go | Adds DeploymentConfig to render options and introduces WithDeploymentConfig. |
| internal/operator-controller/rukpak/render/render_test.go | Adds tests ensuring DeploymentConfig is passed through render options correctly. |
| internal/operator-controller/rukpak/render/registryv1/generators/generators.go | Applies DeploymentConfig to generated CSV Deployment resources via new helper functions. |
| internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go | Adds integration-style tests verifying BundleCSVDeploymentGenerator applies config fields as expected. |
| internal/operator-controller/rukpak/render/registryv1/generators/deployment_config_test.go | Adds unit tests targeting the new config-application helper functions. |
| internal/operator-controller/config/config.go | Introduces a DeploymentConfig alias to v1alpha1.SubscriptionConfig for OLMv1 naming clarity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // envFromEquals checks if two EnvFromSource objects are equal. | ||
| func envFromEquals(a, b corev1.EnvFromSource) bool { | ||
| if a.Prefix != b.Prefix { | ||
| return false | ||
| } | ||
|
|
||
| if (a.ConfigMapRef == nil) != (b.ConfigMapRef == nil) { | ||
| return false | ||
| } | ||
| if a.ConfigMapRef != nil && b.ConfigMapRef != nil { | ||
| if a.ConfigMapRef.Name != b.ConfigMapRef.Name { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| if (a.SecretRef == nil) != (b.SecretRef == nil) { | ||
| return false | ||
| } | ||
| if a.SecretRef != nil && b.SecretRef != nil { | ||
| if a.SecretRef.Name != b.SecretRef.Name { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
envFromEquals() treats two EnvFromSource entries as duplicates based only on Prefix and the referenced ConfigMap/Secret name. This ignores fields like ConfigMapRef.Optional / SecretRef.Optional, so a config that differs only by Optional will be considered a duplicate and skipped (making it impossible to apply that change). Consider including the Optional fields (and any other relevant fields on EnvFromSource) in the equality check, or using a semantic deep-equality comparison for EnvFromSource.
|
|
||
| // applyCustomConfigToDeployment applies the deployment configuration to all containers in the deployment. | ||
| // It follows OLMv0 behavior for applying configuration to deployments. | ||
| // See https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/controller/operators/catalog/subscription/config_types.go |
There was a problem hiding this comment.
The comment link for OLMv0 behavior references blob/master, which can drift over time and makes it harder to verify parity. Prefer pinning to a specific commit SHA (as done in nearby comments) to keep the reference stable.
| // See https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/controller/operators/catalog/subscription/config_types.go | |
| // See https://github.com/operator-framework/operator-lifecycle-manager/blob/f6b2d7c35cf1aaba3a0990353d363df238dab28c/pkg/controller/operators/catalog/subscription/config_types.go |
| // If config is nil, no customizations are applied. | ||
| func WithDeploymentConfig(config *config.DeploymentConfig) Option { | ||
| return func(o *Options) { | ||
| o.DeploymentConfig = config |
There was a problem hiding this comment.
WithDeploymentConfig uses a parameter named config, which shadows the imported config package name inside the function body and can be confusing for future edits. Renaming the parameter to deploymentConfig (or similar) avoids this shadowing and improves readability.
| // If config is nil, no customizations are applied. | |
| func WithDeploymentConfig(config *config.DeploymentConfig) Option { | |
| return func(o *Options) { | |
| o.DeploymentConfig = config | |
| // If deploymentConfig is nil, no customizations are applied. | |
| func WithDeploymentConfig(deploymentConfig *config.DeploymentConfig) Option { | |
| return func(o *Options) { | |
| o.DeploymentConfig = deploymentConfig |
| package generators | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This directory’s tests are otherwise written as external tests (e.g., generators_test.go and resources_test.go use package generators_test). Using package generators here introduces a second test package in the same directory and departs from that convention. If you don’t need access to unexported helpers, consider switching this to package generators_test and exercising behavior via the exported generator API.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2469 +/- ##
==========================================
+ Coverage 69.52% 69.91% +0.38%
==========================================
Files 102 102
Lines 8231 8349 +118
==========================================
+ Hits 5723 5837 +114
- Misses 2056 2058 +2
- Partials 452 454 +2
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:
|
|
Lint needs to be fixed. |
2 participants
Description
This PR implements Phase 2 of the Deployment Configuration RFC: extending the OLMv1 bundle renderer to apply
DeploymentConfigcustomizations to operator deployments.Building on the foundation from #2454, this PR enables the renderer to accept and apply deployment configuration when rendering registry+v1 bundles. The implementation follows OLMv0's behavior patterns to ensure compatibility and correctness.
The next PR will wire up the config in the
ClusterExtensioncontroller by parsingspec.install.configto convert toDeploymentConfigand threadDeploymentConfigthrough the controller's render call chainReviewer Checklist