diff --git a/internal/controller/devhubplugincatalog_controller_test.go b/internal/controller/devhubplugincatalog_controller_test.go index eaa42f891..596fa5306 100644 --- a/internal/controller/devhubplugincatalog_controller_test.go +++ b/internal/controller/devhubplugincatalog_controller_test.go @@ -11,6 +11,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -320,12 +321,12 @@ func TestApplyConfigMap_WithPlugins(t *testing.T) { // Create plugins map plugins := make(catalog.PluginMap) plugins["plugin-techdocs"] = model.DynaPlugin{ - Package: "oci://registry.example.com/rhdh/plugin-techdocs:1.0", - Disabled: false, + Package: "oci://registry.example.com/rhdh/plugin-techdocs:1.0", + Enabled: ptr.To(true), } plugins["plugin-kubernetes"] = model.DynaPlugin{ - Package: "oci://registry.example.com/rhdh/plugin-kubernetes:2.0", - Disabled: false, + Package: "oci://registry.example.com/rhdh/plugin-kubernetes:2.0", + Enabled: ptr.To(true), } err := r.applyConfigMap(context.TODO(), plugins) @@ -342,6 +343,8 @@ func TestApplyConfigMap_WithPlugins(t *testing.T) { // Verify dynamic-plugins.yaml was replaced assert.Contains(t, cm.Data, "dynamic-plugins.yaml") dpContent := cm.Data["dynamic-plugins.yaml"] + assert.Contains(t, dpContent, "enabled: true") + assert.NotContains(t, dpContent, "disabled:") // Content should be a ConfigMap YAML var configMap map[string]interface{} diff --git a/pkg/catalog/processor_test.go b/pkg/catalog/processor_test.go index dac816ad2..51be9c342 100644 --- a/pkg/catalog/processor_test.go +++ b/pkg/catalog/processor_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" + "k8s.io/utils/ptr" "github.com/redhat-developer/rhdh-operator/pkg/model" ) @@ -161,8 +162,8 @@ func TestBuildPatch(t *testing.T) { plugins := make(PluginMap) plugins["plugin-test"] = model.DynaPlugin{ - Package: "oci://registry.example.com/rhdh/plugin-test:1.0", - Disabled: false, + Package: "oci://registry.example.com/rhdh/plugin-test:1.0", + Enabled: ptr.To(true), } patchBytes, err := p.BuildPatch(plugins) @@ -177,6 +178,8 @@ func TestBuildPatch(t *testing.T) { // Verify the content is a ConfigMap YAML dpContent := data[model.DynamicPluginsFile].(string) + assert.Contains(t, dpContent, "enabled: true") + assert.NotContains(t, dpContent, "disabled:") var configMap map[string]interface{} require.NoError(t, yaml.Unmarshal([]byte(dpContent), &configMap)) diff --git a/pkg/model/dynamic-plugins.go b/pkg/model/dynamic-plugins.go index f2eef1395..2721eb2d8 100644 --- a/pkg/model/dynamic-plugins.go +++ b/pkg/model/dynamic-plugins.go @@ -55,11 +55,13 @@ type DynaPluginsConfig struct { Plugins []DynaPlugin `yaml:"plugins,omitempty"` } +// DynaPlugin represents a dynamic plugin entry. Disabled is deprecated; nil +// means the legacy key was not supplied in YAML. type DynaPlugin struct { Package string `yaml:"package,omitempty"` Integrity string `yaml:"integrity,omitempty"` Enabled *bool `yaml:"enabled,omitempty"` - Disabled bool `yaml:"disabled"` + Disabled *bool `yaml:"disabled,omitempty"` PluginConfig map[string]interface{} `yaml:"pluginConfig,omitempty"` Dependencies []PluginDependency `yaml:"dependencies,omitempty"` } @@ -254,7 +256,7 @@ func (p DynaPlugin) IsDisabled() bool { if p.Enabled != nil { return !*p.Enabled } - return p.Disabled + return p.Disabled != nil && *p.Disabled } // Dependencies returns a list of plugin dependencies @@ -419,13 +421,18 @@ func MergePluginsData(firstData, secondData string) (string, error) { } if plugin.Enabled != nil { existingPlugin.Enabled = plugin.Enabled - } else if plugin.Disabled { - existingPlugin.Disabled = true + // Drop a deprecated key inherited from the base, but preserve + // one explicitly declared alongside enabled in the overlay. + existingPlugin.Disabled = plugin.Disabled + } else if plugin.Disabled != nil { + // Preserve an explicitly supplied disabled value, including false. + existingPlugin.Disabled = plugin.Disabled existingPlugin.Enabled = nil } else { // User added this plugin to overlay without specifying enabled/disabled // Default to enabled existingPlugin.Enabled = ptr.To(true) + existingPlugin.Disabled = nil } pluginMap[plugin.Package] = existingPlugin } else { diff --git a/pkg/model/dynamic-plugins_test.go b/pkg/model/dynamic-plugins_test.go index cc5a0c01d..48ef96eea 100644 --- a/pkg/model/dynamic-plugins_test.go +++ b/pkg/model/dynamic-plugins_test.go @@ -489,8 +489,24 @@ includes: assert.NoError(t, err) // Validate that the marshalled string omits empty fields assert.NotContains(t, string(marshalledE), "integrity", "The string should not contain 'integrity:'") - // Validate that the marshalled string always includes disabled field - assert.Contains(t, string(marshalledE), "disabled", "The string should contain 'disabled:'") + // Do not generate deprecated disabled: false for entries without that key. + assert.NotContains(t, string(marshalledE), "disabled:") +} + +func TestDynaPluginDisabledYAMLRoundTrip(t *testing.T) { + for _, input := range []string{ + "package: p\n", + "package: p\nenabled: true\n", + "package: p\ndisabled: false\n", + "package: p\ndisabled: true\n", + "package: p\nenabled: true\ndisabled: true\n", + } { + var plugin DynaPlugin + assert.NoError(t, yaml.Unmarshal([]byte(input), &plugin)) + output, err := yaml.Marshal(plugin) + assert.NoError(t, err) + assert.YAMLEq(t, input, string(output)) + } } func TestIsDisabled(t *testing.T) { @@ -503,12 +519,12 @@ func TestIsDisabled(t *testing.T) { expected bool }{ {"neither set defaults to enabled", DynaPlugin{Package: "p"}, false}, - {"disabled: true", DynaPlugin{Package: "p", Disabled: true}, true}, - {"disabled: false", DynaPlugin{Package: "p", Disabled: false}, false}, + {"disabled: true", DynaPlugin{Package: "p", Disabled: &true_}, true}, + {"disabled: false", DynaPlugin{Package: "p", Disabled: &false_}, false}, {"enabled: true", DynaPlugin{Package: "p", Enabled: &true_}, false}, {"enabled: false", DynaPlugin{Package: "p", Enabled: &false_}, true}, - {"enabled takes precedence over disabled", DynaPlugin{Package: "p", Enabled: &true_, Disabled: true}, false}, - {"enabled: false takes precedence over disabled: false", DynaPlugin{Package: "p", Enabled: &false_, Disabled: false}, true}, + {"enabled takes precedence over disabled", DynaPlugin{Package: "p", Enabled: &true_, Disabled: &true_}, false}, + {"enabled: false takes precedence over disabled: false", DynaPlugin{Package: "p", Enabled: &false_, Disabled: &false_}, true}, } for _, tt := range tests { @@ -565,6 +581,7 @@ plugins: plugin := findPluginByPackage(config.Plugins, "./plugin-a") assert.NotNil(t, plugin) assert.False(t, plugin.IsDisabled(), "enabled: true should override disabled: true") + assert.NotContains(t, merged, "disabled:", "do not emit a synthetic legacy key alongside enabled") }) // Overlay omits both fields — enabled by default @@ -593,6 +610,30 @@ plugins: assert.Equal(t, "sha256-overridden", plugin.Integrity) }) + t.Run("explicit both-key overlay survives the merge", func(t *testing.T) { + base := "plugins:\n - package: ./plugin-a\n disabled: true\n" + overlay := "plugins:\n - package: ./plugin-a\n enabled: false\n disabled: false\n" + merged, err := MergePluginsData(base, overlay) + assert.NoError(t, err) + assert.Contains(t, merged, "enabled: false") + assert.Contains(t, merged, "disabled: false") + var config DynaPluginsConfig + assert.NoError(t, yaml.Unmarshal([]byte(merged), &config)) + assert.True(t, config.Plugins[0].IsDisabled(), "enabled still wins") + }) + + t.Run("explicit disabled false overlay survives the merge", func(t *testing.T) { + base := "plugins:\n - package: ./plugin-a\n enabled: false\n" + overlay := "plugins:\n - package: ./plugin-a\n disabled: false\n" + merged, err := MergePluginsData(base, overlay) + assert.NoError(t, err) + assert.Contains(t, merged, "disabled: false") + assert.NotContains(t, merged, "enabled:") + var config DynaPluginsConfig + assert.NoError(t, yaml.Unmarshal([]byte(merged), &config)) + assert.False(t, config.Plugins[0].IsDisabled()) + }) + // Pure legacy config still works t.Run("legacy disabled-only config works", func(t *testing.T) { base := ` @@ -611,6 +652,7 @@ plugins: pluginA := findPluginByPackage(config.Plugins, "./plugin-a") assert.False(t, pluginA.IsDisabled()) + assert.Contains(t, merged, "disabled: false", "preserve explicit legacy input") pluginB := findPluginByPackage(config.Plugins, "./plugin-b") assert.True(t, pluginB.IsDisabled()) }) diff --git a/pkg/model/testdata/minimal-dynamic-plugins.yaml b/pkg/model/testdata/minimal-dynamic-plugins.yaml index a9b536b36..9cfe2c4f5 100644 --- a/pkg/model/testdata/minimal-dynamic-plugins.yaml +++ b/pkg/model/testdata/minimal-dynamic-plugins.yaml @@ -7,8 +7,8 @@ data: plugins: # Minimal catalog for testing ref:// resolution - package: oci://quay.io/backstage/backstage-plugin-kubernetes@sha256:test123 - disabled: false + enabled: true - package: oci://quay.io/backstage/backstage-plugin-scaffolder-backend-module-github@sha256:test456 - disabled: false + enabled: true - package: oci://quay.io/backstage/backstage-plugin-techdocs@sha256:test789 enabled: true