feat: implicit container contributions

pkg/library/flatten/flatten_test.go

@@ -234,6 +234,29 @@ func TestMergeContainerContributions(t *testing.T) {
 	}
 }
 
+func TestMergeImplicitContainerContributions(t *testing.T) {
+	tests := testutil.LoadAllTestsOrPanic(t, "testdata/implicit-container-contributions")
+	for _, tt := range tests {
+		t.Run(tt.Name, func(t *testing.T) {
+			// sanity check: input defines components
+			assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components")
+			testResolverTools := getTestingTools(tt.Input, "test-ignored")
+
+			outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, nil, testResolverTools)
+			if tt.Output.ErrRegexp != nil && assert.Error(t, err) {
+				assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match")
+			} else {
+				if !assert.NoError(t, err, "Should not return error") {
+					return
+				}
+				assert.Truef(t, cmp.Equal(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts),
+					"DevWorkspace should match expected output:\n%s",
+					cmp.Diff(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts))
+			}
+		})
+	}
+}
+
 func TestMergeSpecContributions(t *testing.T) {
 	tests := testutil.LoadAllTestsOrPanic(t, "testdata/spec-contributions")
 	for _, tt := range tests {
@@ -255,6 +278,27 @@ func TestMergeSpecContributions(t *testing.T) {
 	}
 }
 
+func TestMergeImplicitSpecContributions(t *testing.T) {
+	tests := testutil.LoadAllTestsOrPanic(t, "testdata/spec-contributions")
+	for _, tt := range tests {
+		t.Run(tt.Name, func(t *testing.T) {
+			testResolverTools := getTestingTools(tt.Input, "test-namespace")
+
+			outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, tt.Input.Contributions, testResolverTools)
+			if tt.Output.ErrRegexp != nil && assert.Error(t, err) {
+				assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match")
+			} else {
+				if !assert.NoError(t, err, "Should not return error") {
+					return
+				}
+				assert.Truef(t, cmp.Equal(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts),
+					"DevWorkspace should match expected output:\n%s",
+					cmp.Diff(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts))
+			}
+		})
+	}
+}
+
 func getTestingTools(input testutil.TestInput, testNamespace string) ResolverTools {
 	testHttpGetter := &testutil.FakeHTTPGetter{
 		DevfileResources:      input.DevfileResources,

pkg/library/flatten/merge.go

@@ -124,10 +124,17 @@ func mergeVolume(into, from *dw.VolumeComponent) error {
 }
 
 // needsContainerContributionMerge returns whether merging container contributions is necessary for this workspace. Merging
-// is necessary if at least one component has the merge-contribution: true attribute and at least one component has the
-// container-contribution: true attribute. If either attribute is present but cannot be parsed as a bool, an error is returned.
+// is necessary if the following two conditions are met:
+//
+// - At least one component has the container-contribution: true attribute.
+//
+// - At least one component has the merge-contribution: true attribute OR there exists a container component that was not imported by a
+// plugin or parent devworkspace.
+//
+// If the container-contribution or merge-contribution attribute are present but cannot be parsed as a bool, an error is returned.
+// If multiple components have the merge-contribution: true attribute, an error is returned.
 func needsContainerContributionMerge(flattenedSpec *dw.DevWorkspaceTemplateSpec) (bool, error) {
-	hasContribution, hasTarget := false, false
+	hasContribution, hasTarget, explicitTarget := false, false, false
 	var errHolder error
 	for _, component := range flattenedSpec.Components {
 		if component.Container == nil {
@@ -143,14 +150,23 @@ func needsContainerContributionMerge(flattenedSpec *dw.DevWorkspaceTemplateSpec)
 				// Don't include error in message as it will be propagated to user and is not very clear (references Go unmarshalling)
 				return false, fmt.Errorf("failed to parse %s attribute on component %s as true or false", constants.ContainerContributionAttribute, component.Name)
 			}
-		}
-		if component.Attributes.Exists(constants.MergeContributionAttribute) {
+		} else if component.Attributes.Exists(constants.MergeContributionAttribute) {
+			// Explicit opt out case is handled here if the merge-contributions attribute is set to false
 			if component.Attributes.GetBoolean(constants.MergeContributionAttribute, &errHolder) {
+				if explicitTarget {
+					return false, fmt.Errorf("multiple components have the %s attribute set to true. Only a single component may have the %s attribute set to true", constants.MergeContributionAttribute, constants.MergeContributionAttribute)
+				}
+				explicitTarget = true
 				hasTarget = true
 			}
 			if errHolder != nil {
 				return false, fmt.Errorf("failed to parse %s attribute on component %s as true or false", constants.MergeContributionAttribute, component.Name)
 			}
+		} else {
+			if !component.Attributes.Exists(constants.PluginSourceAttribute) {
+				// First, non-imported container component is implicitly selected as a contribution target
+				hasTarget = true
+			}
 		}
 	}
 	return hasContribution && hasTarget, nil
@@ -164,6 +180,11 @@ func mergeContainerContributions(flattenedSpec *dw.DevWorkspaceTemplateSpec) err
 		}
 	}
 
+	targetComponentName, err := findMergeTarget(flattenedSpec)
+	if err != nil {
+		return err
+	}
+
 	var newComponents []dw.Component
 	mergeDone := false
 	for _, component := range flattenedSpec.Components {
@@ -174,7 +195,7 @@ func mergeContainerContributions(flattenedSpec *dw.DevWorkspaceTemplateSpec) err
 		if component.Attributes.GetBoolean(constants.ContainerContributionAttribute, nil) {
 			// drop contributions from updated list as they will be merged
 			continue
-		} else if component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) && !mergeDone {
+		} else if component.Name == targetComponentName && !mergeDone {
 			mergedComponent, err := mergeContributionsInto(&component, contributions)
 			if err != nil {
 				return fmt.Errorf("failed to merge container contributions: %w", err)
@@ -193,6 +214,52 @@ func mergeContainerContributions(flattenedSpec *dw.DevWorkspaceTemplateSpec) err
 	return nil
 }
 
+// Finds a component that is a suitable merge target for container contributions and returns its name.
+// The following rules are followed when finding a merge target:
+//
+// - A container component that has the merge-contribution: true attribute will automatically be selected as a merge target.
+//
+// - A container component that has the merge-contribution: false attribute will be never be selected as a merge target.
+//
+// - Otherwise, the first container component found that was not imported by a plugin or parent devworkspace (i.e. the controller.devfile.io/imported-by attribute is not present)
+// will be selected as a merge target.
+//
+// If no suitable merge target is found, an error is returned.
+func findMergeTarget(flattenedSpec *dw.DevWorkspaceTemplateSpec) (mergeTargetComponentName string, err error) {
+	firstComponent := ""
+	for _, component := range flattenedSpec.Components {
+		if component.Container == nil {
+			continue
+		}
+
+		if component.Attributes.Exists(constants.MergeContributionAttribute) {
+			// Check for explicit merge contributtion attribute
+			if component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) {
+				return component.Name, nil
+			}
+			// Don't select components that opt out as a merge contribution target
+			continue
+		}
+
+		// The target must not have been imported by a plugin or parent.
+		if component.Attributes.Exists(constants.PluginSourceAttribute) {
+			continue
+		}
+
+		// There might be other components that explicitly opt in as a merge target,
+		// so don't return immediately
+		if firstComponent == "" {
+			firstComponent = component.Name
+		}
+	}
+
+	if firstComponent != "" {
+		return firstComponent, nil
+	}
+
+	return "", fmt.Errorf("couldn't find any merge contribution target component")
+}
+
 func mergeContributionsInto(mergeInto *dw.Component, contributions []dw.Component) (*dw.Component, error) {
 	if mergeInto == nil || mergeInto.Container == nil {
 		return nil, fmt.Errorf("attempting to merge container contributions into a non-container component")

pkg/library/flatten/testdata/container-contributions/error_multiple-contribution-targets.yaml

@@ -0,0 +1,55 @@
+name: "Return error if multiple components have the merge-contribution attribute set to true"
+
+input:
+  devworkspace:
+    components:
+      - name: test-component
+        attributes:
+          controller.devfile.io/merge-contribution: true
+        container:
+          image: test-image
+          env:
+            - name: TEST_ENVVAR
+              value: TEST_VALUE
+      - name: test-component-2
+        attributes:
+          controller.devfile.io/merge-contribution: true
+        container:
+          image: test-image
+          env:
+            - name: TEST_ENVVAR
+              value: TEST_VALUE
+
+      - name: test-contribution
+        plugin:
+          uri: test-contribution.yaml
+
+  devfileResources:
+    test-contribution.yaml:
+      schemaVersion: 2.1.0
+      metadata:
+        name: test-contribution
+      components:
+      - name: test-contribution
+        attributes:
+          controller.devfile.io/container-contribution: true
+        container:
+          image: contribution-image
+          env:
+            - name: CONTRIB_ENVVAR
+              value: CONTRIB_VALUE
+      - name: unmerged-container
+        container:
+          image: unmerged-container
+      - name: unmerged-volume
+        volume: {}
+      commands:
+      - name: plugin-command
+        apply:
+          component: unmerged-container
+      events:
+        prestart:
+          - plugin-command
+
+output:
+  errRegexp: "multiple components have the controller.devfile.io/merge-contribution attribute set to true. Only a single component may have the controller.devfile.io/merge-contribution attribute set to true"

pkg/library/flatten/testdata/container-contributions/no-op-if-no-target.yaml → pkg/library/flatten/testdata/container-contributions/no-op-if-explicit-opt-out.yaml

@@ -1,11 +1,11 @@
-name: "Adds unmerged elements"
+name: "Opt out of container contribution"
 
 input:
   devworkspace:
     components:
       - name: test-component
-        # attributes:
-        #   controller.devfile.io/merge-contribution: true
+        attributes:
+          controller.devfile.io/merge-contribution: false
         container:
           image: test-image
           env:
@@ -47,6 +47,8 @@ output:
   devworkspace:
     components:
       - name: test-component
+        attributes:
+          controller.devfile.io/merge-contribution: false
         container:
           image: test-image
           env:

pkg/library/flatten/testdata/container-contributions/no-op-if-no-contribution.yaml

@@ -1,4 +1,4 @@
-name: "Adds unmerged elements"
+name: "No op if no contribution"
 
 input:
   devworkspace:

pkg/library/flatten/testdata/implicit-container-contributions/adds-resources.yaml

@@ -0,0 +1,44 @@
+name: "Adds attributes from contribution"
+
+input:
+  devworkspace:
+    components:
+      - name: test-component
+        container:
+          image: test-image
+          memoryLimit: 1Gi
+          memoryRequest: 1000Mi
+          cpuLimit: 1500m
+          cpuRequest: "1"
+      - name: test-contribution
+        plugin:
+          uri: test-contribution.yaml
+
+  devfileResources:
+    test-contribution.yaml:
+      schemaVersion: 2.1.0
+      metadata:
+        name: test-contribution
+      components:
+      - name: test-contribution
+        attributes:
+          controller.devfile.io/container-contribution: true
+        container:
+          image: contribution-image
+          memoryLimit: 512Mi
+          memoryRequest: 1.5G
+          cpuLimit: "0.5"
+          cpuRequest: 500m
+
+output:
+  devworkspace:
+    components:
+      - name: test-component
+        attributes:
+          controller.devfile.io/merged-contributions: "test-contribution"
+        container:
+          image: test-image
+          memoryLimit: 1536Mi
+          memoryRequest: "2548576000" # 1.5G + 1000Mi = 1.5*1000^3 + 1000*1024^2
+          cpuLimit: "2"
+          cpuRequest: 1500m

pkg/library/flatten/testdata/implicit-container-contributions/adds-unmerged-elements.yaml

@@ -0,0 +1,74 @@
+name: "Adds unmerged elements"
+
+input:
+  devworkspace:
+    components:
+      - name: test-component
+        container:
+          image: test-image
+          env:
+            - name: TEST_ENVVAR
+              value: TEST_VALUE
+
+      - name: test-contribution
+        plugin:
+          uri: test-contribution.yaml
+
+  devfileResources:
+    test-contribution.yaml:
+      schemaVersion: 2.1.0
+      metadata:
+        name: test-contribution
+      components:
+      - name: test-contribution
+        attributes:
+          controller.devfile.io/container-contribution: true
+        container:
+          image: contribution-image
+          env:
+            - name: CONTRIB_ENVVAR
+              value: CONTRIB_VALUE
+      - name: unmerged-container
+        container:
+          image: unmerged-container
+      - name: unmerged-volume
+        volume: {}
+      commands:
+      - name: plugin-command
+        apply:
+          component: unmerged-container
+      events:
+        prestart:
+          - plugin-command
+
+output:
+  devworkspace:
+    components:
+      - name: test-component
+        attributes:
+          controller.devfile.io/merged-contributions: "test-contribution"
+        container:
+          image: test-image
+          env:
+            - name: TEST_ENVVAR
+              value: TEST_VALUE
+            - name: CONTRIB_ENVVAR
+              value: CONTRIB_VALUE
+      - name: unmerged-container
+        attributes:
+          controller.devfile.io/imported-by: test-contribution
+        container:
+          image: unmerged-container
+      - name: unmerged-volume
+        attributes:
+          controller.devfile.io/imported-by: test-contribution
+        volume: {}
+    commands:
+      - name: plugin-command
+        attributes:
+          controller.devfile.io/imported-by: test-contribution
+        apply:
+          component: unmerged-container
+    events:
+        prestart:
+          - plugin-command