Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicitly set merge-contribution:true on first template component #993

Closed
Tracked by #21864
l0rd opened this issue Dec 12, 2022 · 3 comments · Fixed by #1013
Closed
Tracked by #21864

Implicitly set merge-contribution:true on first template component #993

l0rd opened this issue Dec 12, 2022 · 3 comments · Fixed by #1013
Milestone

Comments

@l0rd
Copy link
Collaborator

l0rd commented Dec 12, 2022

The problem

Currently both a contribution and the attribute controller.devfile.io/merge-contribution: true are required to properly "inject" an IDE in workspace Pod:

kind: DevWorkspace
(...)
spec:
  contributions:
    - name: editor
      kubernetes:
        name: che-code 
  template:
    components:
      - name: tooling-container
        attributes:
          controller.devfile.io/merge-contribution: true

Considered that:

  • most of the time the template only has one container component or, if there are more than one, the developer wants to use the first container as the dev tooling container.
  • the merge-contribution attribute makes sense in a Che scenario but not in odo or dev console scenarios (yet) so we should not specify it in registry.devfile.io.
  • we want the devworkspace template to match the original devfile so it's easier to update it if the original devfile is updated.

we should define a convention to apply contributions when that's not explicitly specified in the template/devfile.

A proposal

  • opt-in to contribution is implicit: when no component has a merge-contribution attribute, the contribution should be applied to the first container component.
  • out-out to contribution is explicit: the author or a devfile should explicitly set merge-contribution to false to avoid contributions to that component.
  • a devfile with multiple merge-contributions: true is invalid: the devworkspace controller should return an error if multiple components have merge-contribution explicitly set to true.
@amisevsk
Copy link
Collaborator

One grey area with implicitly including the merge-contributions: true attribute is for devfiles that do not explicitly include a container. What will happen in this case is that DWO will attempt to inject into the first container in the resolved (flattened) devfile.

For example, in our default "theia-next" sample, DWO would attempt to inject into the theia-ide component, which may not work as expected (the behavior of DWO as it is now would be to run the theia-ide-contributions container using the default image specified there -- i.e. UDI).

Currently, I believe DWO internally flattens contributions + plugin components into one list of components to start, so this behavior would be present with the theia-next-contributions sample

One of the reasons we defaulted to explicit opt-in, implicit opt-out is to be sure a workspace can be started (assuming the default image for unmerged components is suitable for running the injected component). With implicit injection, adding e.g. a busybox container to the workspace will cause startup to likely fail -- though this may not be a use case we care to support.

There are two solutions I can see here:

  • Make sure we explicitly opt out of merging for plugin components (i.e. set merge-contribution: false on the theia-ide component, etc.)
  • Try to work out a system where DWO only attempts to inject into components (and not contributions) -- this would be hard due to how contributions are treated internally and could break some (deprecated but still possible) use cases.

@l0rd
Copy link
Collaborator Author

l0rd commented Dec 15, 2022

I am not 100% sure I have understood the problem so apologies if what I am saying doesn't make sense. My understanding:

  • The "theia-next" sample doesn't have any contribution so there is no problem.
  • The "theia-next-contributions" sample has a contribution and doesn't have a component. In this case the contribution will be used as component but it may not have been thought to be used like this so it may not work.

From my point of view, DevWorkspaces with no components should be considered invalid. For the Devfile we may want to be more permissive and allow component-less: Che, or any other tool that generates the DW, would be responsible to add the default component (this case is similar to a recent customer case).

@amisevsk
Copy link
Collaborator

From my point of view, DevWorkspaces with no components should be considered invalid

That's a valid option; currently devfiles with no components whatsoever are valid in order to allow for the flow "create a devfile, add components after creation". Additionally, from a "resolved workspace" perspective, the theia-next samples (including contributions) do technically have components, they're just imported from the Theia DevWorkspaceTemplate. We can imagine use cases where this set up is valuable -- for example a plugin/contribution that references a bootstrapping component (that clones the repo and sets up the devfile, in order to support non-github/gitlab/bitbucket repositories).

Regarding

The "theia-next" sample doesn't have any contribution so there is no problem.

In order to support a smoother transition to using contributions instead of plugin components, the container-contribution attribute is respected for both plugin components and contributions, so this workspace will be treated the same way as the contributions sample internally. We can (and perhaps should) change that.

AObuchow added a commit to AObuchow/devworkspace-operator that referenced this issue Jan 5, 2023
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 implcitly selected as a container contribution merge target by setting the `controller.devfile.io/merge-contribution` attributete 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]>
AObuchow added a commit to AObuchow/devworkspace-operator that referenced this issue Jan 5, 2023
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]>
AObuchow added a commit to AObuchow/devworkspace-operator that referenced this issue Jan 5, 2023
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]>
AObuchow added a commit to AObuchow/devworkspace-operator that referenced this issue Jan 9, 2023
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]>
ibuziuk pushed a commit that referenced this issue Jan 10, 2023
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 #993

Signed-off-by: Andrew Obuchowicz <[email protected]>
@amisevsk amisevsk added this to the v0.19.x milestone Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants