feat(pluginpresets): pluginPreset controller takes over the evaluation of references to other Plugins (#1776)#2034
Open
k-fabryczny wants to merge 3 commits into
Open
feat(pluginpresets): pluginPreset controller takes over the evaluation of references to other Plugins (#1776)#2034k-fabryczny wants to merge 3 commits into
k-fabryczny wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR shifts evaluation of PluginOptionValue.Expression and PluginOptionValue.ValueFrom.Ref from the Plugin controller to the PluginPreset controller, so Plugins created from PluginPresets are materialized with fully resolved/static option values (aligning with #1776).
Changes:
- Added PluginPreset-side resolution for CEL expressions and
valueFrom.refreferences (including cross-preset references by name/selector) plus controller wiring via feature flags. - Updated API/CRD/docs to reflect PluginPreset spec shape changes (notably removing
spec.plugin.clusterName) and to mark Plugin Expression/Ref as deprecated for standalone Plugins. - Added/updated unit + controller + E2E tests covering PluginPreset expression evaluation and reference resolution paths.
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook/v1alpha1/pluginpreset_webhook.go | Removes ClusterName validation now that it’s no longer part of the PluginPreset plugin spec. |
| internal/webhook/v1alpha1/pluginpreset_webhook_test.go | Updates webhook tests for the PluginPreset plugin spec type changes. |
| internal/webhook/v1alpha1/plugin_webhook.go | Adds lint suppression around deprecated Expression usage. |
| internal/webhook/v1alpha1/plugin_webhook_test.go | Updates tests to reference deprecated Expression with lint suppression. |
| internal/test/resources.go | Adjusts test helpers for deprecated fields and PluginPreset plugin spec mapping. |
| internal/helm/helm.go | Adds lint suppression for deprecated fields in checksum computation. |
| internal/helm/cel.go | Adds lint suppression for deprecated expression resolver API usage. |
| internal/features/features.go | Adds pluginPreset feature flags and accessors. |
| internal/features/features_test.go | Adds tests for PluginPreset feature flags and independence from Plugin flags. |
| internal/controller/plugin/suite_test.go | Enables PluginPreset controller flags in controller test suite setup. |
| internal/controller/plugin/pluginpreset_values_resolver.go | New resolver implementing expression + reference resolution for PluginPresets. |
| internal/controller/plugin/pluginpreset_controller.go | Integrates override-then-resolve flow and maps PluginPreset spec into Plugin spec. |
| internal/controller/plugin/pluginpreset_controller_test.go | Adds extensive tests for expression and reference resolution + override merge behavior. |
| internal/controller/plugin/plugin_values_resolver.go | Adds lint suppression for deprecated ValueFrom.Ref access. |
| internal/controller/plugin/plugin_controller_flux.go | Adds TODOs and lint suppression for deprecated Plugin-side evaluation branches. |
| internal/cmd/plugin_template_test.go | Updates pluginPreset spec type usage in CLI tests. |
| internal/clientutil/util.go | Refactors cluster deletion filtering using slices.DeleteFunc. |
| e2e/pluginpreset/testdata/organization.yaml | New E2E test fixture for Organization bootstrap. |
| e2e/pluginpreset/scenarios/selector_reference.go | New E2E scenario validating selector-based cross-preset references. |
| e2e/pluginpreset/scenarios/expression_evaluation.go | New E2E scenario validating PluginPreset expression evaluation. |
| e2e/pluginpreset/scenarios/cross_preset_reference.go | New E2E scenario validating name-based cross-preset references. |
| e2e/pluginpreset/e2e_test.go | New E2E suite for PluginPreset expression/ref resolution behavior. |
| docs/reference/components/pluginpreset.md | Documents PluginPreset CEL expressions and cross-preset reference semantics + feature flags. |
| docs/reference/api/openapi.yaml | Updates OpenAPI descriptions and removes PluginPreset clusterName from schema. |
| docs/reference/api/index.html | Updates rendered API docs for new PluginPreset types and deprecation notes. |
| dev-env/dev.values.yaml | Enables pluginPreset feature flags in dev environment values. |
| cmd/greenhouse/controllers.go | Wires PluginPreset reconciler startup with feature flags. |
| charts/manager/templates/manager/feature-flag.yaml | Adds pluginPreset feature flag section to feature-flags ConfigMap. |
| charts/manager/templates/_helpers.tpl | Adds Helm helpers for pluginPreset feature flags. |
| charts/manager/crds/greenhouse.sap_plugins.yaml | Updates CRD descriptions for deprecated standalone Plugin Expression/Ref. |
| charts/manager/crds/greenhouse.sap_pluginpresets.yaml | Updates PluginPreset CRD schema (removes clusterName, updates descriptions). |
| charts/manager/ci/test-values.yaml | Sets pluginPreset feature flags for chart CI values. |
| charts/greenhouse/values.yaml | Adds default pluginPreset feature flags to greenhouse chart values. |
| charts/greenhouse/ci/test-values.yaml | Adds pluginPreset feature flags to greenhouse chart CI values. |
| api/v1alpha1/zz_generated.deepcopy.go | Adds deepcopy functions for newly introduced PluginPreset-related types. |
| api/v1alpha1/pluginpreset_types.go | Introduces PluginPresetPluginSpec and related types; updates PluginPresetSpec.Plugin field type. |
| api/v1alpha1/plugin_types.go | Marks Expression and ValueFrom.Ref as deprecated for standalone Plugins. |
| .golangci.yaml | Tweaks goconst config to ignore tests and short identifier-like strings. |
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+91
to
+95
| // PluginPresetPluginOptionValue is the value for a PluginOption. | ||
| type PluginPresetPluginOptionValue struct { | ||
| // Name of the values. | ||
| Name string `json:"name"` | ||
| // Value is the actual value in plain text. |
Comment on lines
+91
to
+113
| // PluginPresetPluginOptionValue is the value for a PluginOption. | ||
| type PluginPresetPluginOptionValue struct { | ||
| // Name of the values. | ||
| Name string `json:"name"` | ||
| // Value is the actual value in plain text. | ||
| Value *apiextensionsv1.JSON `json:"value,omitempty"` | ||
| // ValueFrom references value in another source. | ||
| ValueFrom *PluginPresetPluginValueFromSource `json:"valueFrom,omitempty"` | ||
| // Expression is a YAML string with ${...} placeholders that will be evaluated as CEL expressions. | ||
| Expression *string `json:"expression,omitempty"` | ||
| } | ||
|
|
||
| // PluginPresetPluginValueFromSource defines how to extract dynamic values | ||
| // only one of secret or ref can be set | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.secret) && has(self.ref))",message="both secret and ref cannot be set" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.secret) || has(self.ref)",message="one of secret or ref must be set" | ||
| type PluginPresetPluginValueFromSource struct { | ||
| // Secret references the v1.Secret containing the value that needs to be extracted | ||
| Secret *SecretKeyReference `json:"secret,omitempty"` | ||
| // Ref references values defined in another resource (Plugin, PluginPreset) | ||
| Ref *ExternalValueSource `json:"ref,omitempty"` | ||
| } | ||
|
|
| DisplayName string `json:"displayName,omitempty"` | ||
|
|
||
| // Values are the values for a PluginDefinition instance. | ||
| OptionValues []PluginOptionValue `json:"optionValues,omitempty"` |
ebc0e14 to
0da5825
Compare
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
0da5825 to
3807e90
Compare
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What type of PR is this? (check all applicable)
Related Tickets & Documents
#1776
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist