feat: implicit container contributions #1013
Conversation
pkg/library/flatten/merge.go
Outdated
| // First check for explicit merge contributtion attribute | ||
| for _, component := range flattenedSpec.Components { | ||
| if component.Container == nil { | ||
| continue | ||
| } | ||
|
|
||
| if component.Attributes.Exists(constants.MergeContributionAttribute) { | ||
| if component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) { | ||
| return component.Name, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Then see if there's a container that can implicitly be selected as a merge target | ||
| for _, component := range flattenedSpec.Components { | ||
| if component.Container == nil { | ||
| continue | ||
| } | ||
|
|
||
| // Don't select components that opt out as a merge contribution target | ||
| if component.Attributes.Exists(constants.MergeContributionAttribute) && !component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) { | ||
| continue | ||
| } | ||
|
|
||
| // The target must not have been imported by a plugin or parent. | ||
| if component.Attributes.Exists(constants.PluginSourceAttribute) { | ||
| continue | ||
| } | ||
| return component.Name, nil | ||
| } |
There was a problem hiding this comment.
This could be simplified into a single pass in a pretty straightforward way:
firstComponent := ""
for _, component := range flattenedSpec.Components {
if component.Container == nil {
continue
}
if component.Attributes.Exists(constants.MergeContributionAttribute) {
if component.Attributes.GetBoolean(constants.MergeContributionAttribute, nil) {
return component.Name, nil
}
}
if component.Attributes.Exists(constants.PluginSourceAttribute) {
continue
}
if firstComponent == "" {
firstComponent = component.Name
}
}There was a problem hiding this comment.
Good call, I added a fixup for this (I included an extra continue to prevent accidentally selecting a component that opts out as a merge contribution target)
Codecov ReportBase: 49.87% // Head: 49.98% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
+ Coverage 49.87% 49.98% +0.11%
==========================================
Files 68 69 +1
Lines 5911 5968 +57
==========================================
+ Hits 2948 2983 +35
- Misses 2736 2759 +23
+ Partials 227 226 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
AObuchow
force-pushed
the
implicit-merge-contribution
branch
from
2d38d67 to
52fb3cd
Compare
|
Added an extra test case to account for the case where the first component could be implicitly selected as a merge target, but a later component explicitly opts in as a merge target. A duplicate test could be added for |
This commit introduces 3 changes to the container contribution mechanism: 1. The `controller.devfile.io/merge-contribution` component attribute is no longer required in order to define a target for container contributions. The first container component in a DevWorkspace (that is not imported by a plugin or parent DevWorkspace) is automatically selected as a merge target if the `controller.devfile.io/container-contribution` is used on at least one other DevWorkspace component. However, if the `controller.devfile.io/merge-contribution` attribute is set to `true` for a container component, it will be selected as a target for container contributions. 2. Users can opt out a container component from being implicitly selected as a container contribution merge target by setting the `controller.devfile.io/merge-contribution` attribute to false. 3. If a DevWorkspace has multiple container components with the `controller.devfile.io/merge-contribution` attribute set to true, the workspace fails. Fix devfile#993 Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Andrew Obuchowicz <[email protected]>
…ner contribution merge target This commit adds a webhook handler to ensure: - A newly created DevWorkspace cannot have more than one container component with the controller.devfile.io/merge-contribution set to true. - An existing DevWorkspace cannot be modified to have more than one container component with the controller.devfile.io/merge-contribution set to true. Signed-off-by: Andrew Obuchowicz <[email protected]>
AObuchow
force-pushed
the
implicit-merge-contribution
branch
from
52fb3cd to
44bc365
Compare
|
Squashed the fixups |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, AObuchow, ibuziuk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?
This PR introduces 3 changes to the container contribution mechanism:
The
controller.devfile.io/merge-contributioncomponent attribute is no longer required in order to define a target for container contributions. The first container component in a DevWorkspace (that is not imported by a plugin or parent DevWorkspace) is automatically selected as a merge target if thecontroller.devfile.io/container-contributionis used on at least one other DevWorkspace component. However, if thecontroller.devfile.io/merge-contributionattribute is set totruefor a container component, it will be selected as a target for container contributions.Users can opt out a container component from being implicitly selected as a container contribution merge target by setting the
controller.devfile.io/merge-contributionattribute to false.If a DevWorkspace has multiple container components with the
controller.devfile.io/merge-contributionattribute set to true, the workspace fails. The webhook was extended to prevent the creation of a DevWorkspace that has multiple container components with thecontroller.devfile.io/merge-contributionattribute set to true, and to prevent updating a DevWorkspace to have multiple container components with themerge-contributionattribute set to true.What issues does this PR fix or reference?
Fix #993
Is it tested? How?
Automated tests
Automated tests were added to check that implicit container contributions (with the
spec.contributionsfield and the attribute) work as expected; these are based off of the tests previously created in #844 and #939. A test was also added to ensure that DevWorkspaces with multiple container components with thecontroller.devfile.io/merge-contribution: trueattribute will fail.Manual testing
First, checkout the changes from this PR and install DWO onto your cluster, e.g set your DWO_IMG environment variable to point to your quay repo and
make docker installon OpenShift.The following DevWorkspaces were based off of the Che-Theia + Golang example from #844. They should behave the same as the original example, except for the opt-out cases and multiple
merge-contributionattribute cases.Ensure implicit container contributions work
Apply the following DevWorkspace:
When inspecting the
toolscontainer, you should see that contributions from thetheia-ideplugin component have been added, e.g. thePLUGIN_REMOTE_ENDPOINT_EXECUTABLEenvironment variable is set.Ensure implicit container contributions work with
spec.contributionsApply the following DevWorkspace, and similar to the last test case, ensure you can see contributions from
theia-ide(e.g. environment variables)Ensure opting out from implicit container contributions works
Apply the following DevWorkspace. It has 2 container components, with the first one (
opt-out) explicitly opting out from being a container contribution target. Ensure that theopt-outcontainer does not have contributions fromtheia-ide, and that thetoolscontainer does have the contributions.Ensure DevWorkspaces with multiple container components that have the
merge-contribution: trueattribute are rejectedApply the following DevWorkspace:
You should get a response similar to the following:
Ensure DevWorkspaces modifications to make multiple container components have the
merge-contribution: trueattribute are rejectedApply the following DevWorkspace:
Now do a
kubectl edit dw test-contrib-theia-add-multiple-merge-contributions -n $NAMESPACEto add an extramerge-contribution: trueattribute to thetools-2componentkind: DevWorkspace apiVersion: workspace.devfile.io/v1alpha2 metadata: name: test-contrib-theia-add-multiple-merge-contributions spec: (...) - name: tools-2 attributes: + controller.devfile.io/merge-contribution: true che-theia.eclipse.org/vscode-extensions: - https://github.com/golang/vscode-go/releases/download/v0.23.0/go-0.23.0.vsix container: image: quay.io/devfile/universal-developer-image:ubi8-0e189d9 memoryLimit: 2Gi mountSources: true (...)You should get an error mentioning that multiple container components have the
merge-contribution: true.PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che