chore(plugins): remove unused code#2102
Open
k-fabryczny wants to merge 16 commits into
Open
Conversation
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Cleanup PR intended to remove now-unused plugin-side CEL/external reference resolution after moving evaluation responsibilities to PluginPreset.
Changes:
- Removed
ValueFrom.Refresolution helpers (including CEL evaluation and tracking annotation updates) fromplugin_values_resolver.go. - Deleted unit/integration tests that covered the removed helper behavior.
- Simplified
computeReleaseValuesin the Flux plugin controller path by removing expression/ref resolution logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/controller/plugin/plugin_values_resolver.go | Removes external reference/CEL resolution and related tracking-annotation helpers, leaving only untracking cleanup logic. |
| internal/controller/plugin/plugin_values_resolver_test.go | Removes unit tests that covered the deleted helper functions. |
| internal/controller/plugin/plugin_integration/plugin_integration_test.go | Removes integration specs covering external ref resolution + tracking behavior (file now effectively has only setup/cleanup). |
| internal/controller/plugin/plugin_controller_flux.go | Drops expression/ref handling from computeReleaseValues and keeps integration tracking cleanup flow. |
Comments suppressed due to low confidence (1)
internal/controller/plugin/plugin_controller_flux.go:482
computeReleaseValuesno longer handles option values that useExpressionorValueFrom.Ref: the loop now returnsoption value %s has no value or valueFrom setfor those cases, even though these fields are still part ofPluginOptionValue(and are used elsewhere, e.g. option checksum calculation and e2e plugin integration scenarios). This will break reconciliation for Plugins that rely on expression evaluation or external references, and the error message is misleading.
Either restore expression/ref resolution here (or earlier in helm.GetPluginOptionValuesForPlugin), or explicitly reject these fields with a clear error and update validation/webhooks + tests accordingly.
for _, v := range optionValues {
switch {
case v.Value != nil:
// noop, direct values are already set
continue
case v.ValueFrom != nil && v.ValueFrom.Secret != nil:
// noop, secret refs are not resolved here
continue
default:
return nil, fmt.Errorf("option value %s has no value or valueFrom set", v.Name)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Zaggy21
requested changes
Jul 1, 2026
| // computeReleaseValues resolves Expressions and ValueFromRefs in the Plugin's option values | ||
| // and inserts the Greenhouse values | ||
| func computeReleaseValues(ctx context.Context, c client.Client, plugin *greenhousev1alpha1.Plugin, expressionEvaluation, integrationEnabled bool) ([]greenhousev1alpha1.PluginOptionValue, error) { | ||
| func computeReleaseValues(ctx context.Context, c client.Client, plugin *greenhousev1alpha1.Plugin, _expressionEvaluation, _integrationEnabled bool) ([]greenhousev1alpha1.PluginOptionValue, error) { |
Contributor
There was a problem hiding this comment.
Please remove the unused parameters along with the ExpressionEvaluationEnabled and IntegrationEnabled flags from PluginReconciler, Plugin configuration and other places.
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/features/features_test.go:216
Test_PluginPresetFeaturesno longer covers the common cases whereexpressionEvaluationEnabledis explicitlyfalseor where thepluginPresetkey exists but does not includeexpressionEvaluationEnabled. Keeping these cases helps ensure the default/disabled behavior remains stable (and distinct from the ConfigMap-not-found path).
testCases := []testCase{
{
name: "it should return true when expressionEvaluationEnabled is true",
configMapData: map[string]string{PluginPresetFeatureKey: "expressionEvaluationEnabled: true\n"},
expectedExpressionEvaluation: true,
},
{
name: "it should return false when feature-flags cm is not found",
getError: apierrors.NewNotFound(schema.GroupResource{}, "configmap not found"),
expectedExpressionEvaluation: false,
},
{
name: "it should return false when flag is malformed in feature-flags cm",
configMapData: map[string]string{PluginPresetFeatureKey: "expressionEvaluationEnabled:: invalid_yaml"},
expectedExpressionEvaluation: false,
},
}
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Comment on lines
84
to
+88
| func startPluginReconciler(name string, mgr ctrl.Manager) error { | ||
| return (&plugincontrollers.PluginReconciler{ | ||
| KubeRuntimeOpts: kubeClientOpts, | ||
| ExpressionEvaluationEnabled: featureFlags.IsExpressionEvaluationEnabled(), | ||
| IntegrationEnabled: featureFlags.IsIntegrationEnabled(), | ||
| OCIMirroringEnabled: featureFlags.IsOCIMirroringEnabled(), | ||
| StoragePath: artifactStoragePath, | ||
| HTTPRetry: artifactRetries, | ||
| KubeRuntimeOpts: kubeClientOpts, | ||
| OCIMirroringEnabled: featureFlags.IsOCIMirroringEnabled(), | ||
| StoragePath: artifactStoragePath, |
Comment on lines
476
to
+480
| case v.ValueFrom != nil && v.ValueFrom.Secret != nil: | ||
| // noop, secret refs are not resolved here | ||
| continue | ||
| default: | ||
| return nil, fmt.Errorf("option value %s has no value or valueFrom set", v.Name) | ||
| } | ||
| } | ||
|
|
||
| // update tracking information for plugin integrations | ||
| if integrationEnabled { | ||
| // remove tracking annotations from resources that are no longer being tracked | ||
| if err := removeUntrackedObjectAnnotations(ctx, c, plugin, trackedObjects); err != nil { | ||
| // log err, will retry on next reconciliation | ||
| log.FromContext(ctx).Error(err, "failed to remove untracked object annotations", "namespace", plugin.Namespace, "plugin", plugin.Name) | ||
| } | ||
| if len(trackedObjects) > 0 { | ||
| plugin.Status.TrackedObjects = trackedObjects | ||
| } else { | ||
| // clear tracked objects if there are none | ||
| plugin.Status.TrackedObjects = nil | ||
| return nil, fmt.Errorf("plugin %s/%s: option value %s must have a direct value or secret reference", |
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
github-actions
Bot
added
documentation
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Comment on lines
+307
to
312
| // TODO restore | ||
| //nolint:dupword | ||
| // Entry("Expression only (valid)", nil, nil, utils.StringP(`"test-${global.greenhouse.clusterName}"`), false), | ||
| // Entry("Expression and Value both set (invalid)", test.MustReturnJSONFor("test"), nil, utils.StringP(`"test-expression"`), true), | ||
| Entry("Expression and ValueFrom both set (invalid)", nil, &greenhousev1alpha1.PluginPresetPluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret"}}, utils.StringP(`"test-expression"`), true), | ||
| Entry("All three set (invalid)", test.MustReturnJSONFor("test"), &greenhousev1alpha1.PluginPresetPluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret"}}, utils.StringP(`"test-expression"`), true), |
Comment on lines
+391
to
396
| // TODO restore | ||
| //nolint:dupword | ||
| // Entry("Expression only (valid)", nil, nil, utils.StringP(`"test-${global.greenhouse.clusterName}"`), false), | ||
| // Entry("Expression and Value both set (invalid)", test.MustReturnJSONFor("test"), nil, utils.StringP(`"test-expression"`), true), | ||
| Entry("Expression and ValueFrom both set (invalid)", nil, &greenhousev1alpha1.PluginPresetPluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret"}}, utils.StringP(`"test-expression"`), true), | ||
| Entry("All three set (invalid)", test.MustReturnJSONFor("test"), &greenhousev1alpha1.PluginPresetPluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret"}}, utils.StringP(`"test-expression"`), true), |
Comment on lines
389
to
394
| // hasExactlyOneValueSource checks if exactly one of Value, ValueFrom, or Expression is set. | ||
| func hasExactlyOneValueSource(val greenhousev1alpha1.PluginOptionValue) bool { | ||
| sources := []bool{ | ||
| val.Value != nil, | ||
| val.ValueFrom != nil, | ||
| val.Expression != nil, | ||
| } |
Comment on lines
136
to
140
| ov := greenhousev1alpha1.PluginOptionValue{ | ||
| Name: pv.Name, | ||
| Value: pv.Value, | ||
| Expression: pv.Expression, | ||
| Name: pv.Name, | ||
| Value: pv.Value, | ||
| } | ||
| if pv.ValueFrom != nil { |
Comment on lines
+26
to
+30
| DescribeTable("Validate PluginType contains either Value or ValueFrom", func(value *apiextensionsv1.JSON, valueFrom *greenhousev1alpha1.PluginValueFromSource, expression *string, expErr bool) { | ||
| optionValues := []greenhousev1alpha1.PluginOptionValue{ | ||
| { | ||
| Name: "test", | ||
| Value: value, | ||
| ValueFrom: valueFrom, | ||
| Expression: expression, | ||
| Name: "test", | ||
| Value: value, |
Comment on lines
71
to
76
| Entry("Value and ValueFrom and Expression nil", nil, nil, nil, true), | ||
| Entry("Value and ValueFrom not nil, Expression is nil", test.MustReturnJSONFor("test"), &greenhousev1alpha1.PluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret"}}, nil, true), | ||
| Entry("Value not nil", test.MustReturnJSONFor("test"), nil, nil, false), | ||
| Entry("ValueFrom not nil", nil, &greenhousev1alpha1.PluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret", Key: "secret-key"}}, nil, false), | ||
| Entry("Expression not nil", nil, nil, new("${global.greenhouse.clusterName}"), false), | ||
| Entry("Value and Expression not nil, ValueFrom nil", test.MustReturnJSONFor("test"), nil, new("${global.greenhouse.clusterName}"), true), | ||
| Entry("ValueFrom and Expression not nil, Value nil", nil, &greenhousev1alpha1.PluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret", Key: "secret-key"}}, new("${global.greenhouse.clusterName}"), true), | ||
| Entry("Value, ValueFrom, and Expression all not nil", test.MustReturnJSONFor("test"), &greenhousev1alpha1.PluginValueFromSource{Secret: &greenhousev1alpha1.SecretKeyReference{Name: "my-secret", Key: "secret-key"}}, new("${global.greenhouse.clusterName}"), true), | ||
| ) |
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
Cleanup after moving CEL evaluation from plugin to PluginPreset
What type of PR is this? (check all applicable)
Related Tickets & Documents
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