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

Polymorphic component type UX in a devfile #4

Closed
l0rd opened this issue Feb 4, 2020 · 11 comments
Closed

Polymorphic component type UX in a devfile #4

l0rd opened this issue Feb 4, 2020 · 11 comments
Labels
area/api Enhancement or issue related to the api/devfile specification kind/enhancement New feature or request
Milestone

Comments

@l0rd
Copy link
Contributor

l0rd commented Feb 4, 2020

Polymorphic types makes devWorkspaces objects easier to validate and more close as a Kubernetes custom resources. But are we sacrificing the UX of the devfile? Was the 1.0.0 devfile spec easier to read and modify for humans?

devfile 1.0.0

components:
  - type: container
    image: maven
      ...
  - type: container
    image: nodejs
      ...
  - type: kubernetes
    reference: https://gist.../mongodb.yaml
       ...

current proposal

components:
  - container:
        image: maven
        ...
  - container:
        image: nodejs
        ...
  - kubernetes:
       reference: https://gist.../mongodb.yaml
       ...

alternative proposal

components:
  containers:
    - image: maven
        ...
    - image: nodejs
        ...
  kubernetes:
    - reference: https://gist.../mongodb.yaml
    ...
@l0rd l0rd changed the title Polymorphic component and command types in the devfile Polymorphic component type UX in a devfile Feb 4, 2020
@l0rd l0rd added kind/enhancement New feature or request area/api Enhancement or issue related to the api/devfile specification labels Feb 4, 2020
@johnmcollier
Copy link
Member

I prefer the alternative proposal. Having each type (containers, kubernetes, etc) under component be an array of elements (of length 0, 1, or many), makes things easier to read / understand in my opinion

@joshuawilson
Copy link

I was thinking of it a little bit different from the alt version.

Current

components:
  - chePlugin:
      registry:
        id: redhat/vscode-yaml/latest
  - chePlugin:
      registry:
        id: ms-vscode/go/latest
  - cheEditor:
      registry:
        id: eclipse/che-theia/latest
        registryUrl: "external-registry-url"
  - container:
      image: some container image with required build tools
      name: "build-tools" 

Alternative 2

components:
  - component:
      type: chePlugin
      registry:
        id: redhat/vscode-yaml/latest
  - component:
      type: chePlugin
      registry:
        id: ms-vscode/go/latest
  - component:
      type: cheEditor
      registry:
        id: eclipse/che-theia/latest
        registryUrl: "external-registry-url"
  - component:
      type: container
      image: some container image with required build tools
      name: "build-tools" 

Alt 2 provides a more extensible model. If we use the current proposed schema, then any change would need to be done in Go code. However, I can see how this may take more work to validate.

I do not think the language should drive the data structure but it can inform how it is made.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 5, 2020

@joshuawilson this looks redundant:

components:
  - component:
      type: ...

I would rather have the component spec directly without prefixing it with component::

components:
  - type: ...

And that corresponds to the devfile 1.0.0 format (and if there is an agreement that this is the best alternative I am perfectly fine with it).

@l0rd l0rd mentioned this issue Feb 5, 2020
28 tasks
@davidfestal
Copy link
Collaborator

@joshuawilson afaict, the following alt2 syntax proposal ...

components:
  - component:
      type: chePlugin
      registry:
        id: redhat/vscode-yaml/latest
  - component:
      type: cheEditor
      registry:
        id: eclipse/che-theia/latest
        registryUrl: "external-registry-url"
  - component:
      type: container
      image: some container image with required build tools
      name: "build-tools" 

... seems to move the problem encountered with the old Devfile 1.0.0 syntax, to a lower level (under the intermediate component level):

You may have distinct sub-schemas (according to the type field) under the same field name (component), which makes declarative validation (validation by the schema itself) harder it seems and, afaik, makes it very difficult to serialize/unserialize in some languages that don't have polymorphic subtyping (like GO).

OTOH, the whole idea of devfile 2.0.0 current proposal is to avoid having different sub-schemas under the same field name, in a way that would be at the same time:

  • nice for the human end-user,
  • and the recommended way to express polymorphic fields GO-wise and K8S-wise.

Some pointers about the context that drove this initial design;

@davidfestal
Copy link
Collaborator

I prefer the alternative proposal. Having each type (containers, kubernetes, etc) under component be an array of elements (of length 0, 1, or many), makes things easier to read / understand in my opinion

I assume this adds a constraint on the order in which components have to be described, since they would be gathered by type ?

@metlos
Copy link

metlos commented Feb 10, 2020

I am not sure if it is a concern of the current effort for Devfile 2.0 but IMHO the "clustering" by component type, reflected in the schema, makes it kinda hard for the devfile spec to be interoperable, e.g. reused by other "processors" than Che/ODO without the need to add explicit support for them in the source code.

E.g. in my dreamt up world, I would like to support multiple IDEs with a single devfile, maybe using something like this:

components:
- type: chePlugin
  id: redhat/java/latest
- type: eclipseDesktopPlugin
  id: org.eclipse.jdt
- type: gitpodFeature
  id: java-support 
...

Is that something we want to address in any way or is it out of scope?

Or, given the majority of examples above, are we maybe considering externalizing the IDE configuration into some other part of the devfile than components - something in support of #5?

@davidfestal
Copy link
Collaborator

@metlos In your example:

components:
- type: chePlugin
  id: redhat/java/latest
- type: eclipseDesktopPlugin
  id: org.eclipse.jdt
- type: gitpodFeature
  id: java-support 

the 3 components are in fact structurally equivalent, have the same sub-schema, and thus to me they seem to be the same type of component.

The fact that they would define different types of plugin would probably fit into the encapsulated plugin definition itself (currently the meta.yaml format in devfile.1.0.0).
Sure we initially called this component type chePlugin, but in fact if you look into the meta.yaml format it already covers 3 flavors of plugins: pure Che plugin (cf che-machine-exec, theia editor), Theia plugin or VsCode plugin.

So it seems to me that, while discussing this matter, we should avoid mixing component type, which drives distinct component sub-schemas, and what a plugin type field could be, i.e. a way to provide more insight about how a given plugin should be installed / used.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 14, 2020

@metlos that's an interesting scenario and I think we should take it in consideration. Your sample with the current proposal would look like:

components:
  - chePlugin:
        id: redhat/java/latest
  - eclipseDesktopPlugin:
        id: org.eclipse.jdt
  - gitpodFeature:
       id: java-support 
...

or, if I am interpreting @davidfestal answer correctly:

components:
  - plugin:
        id: redhat/java/latest
  - plugin:
        id: desktopEclipse/org.eclipse.jdt
  - plugin:
       id: gitpodFeature/java-support 
...

What's wrong with those?

@metlos
Copy link

metlos commented Feb 14, 2020

What I didn't take into consideration and what we discussed with @davidfestal before he posted his answer here was that the distinction between the different plugin types (e.g. theia/eclipse/gitpod/codewind/...) can happen in meta.yaml of the plugins and that all we then need is to be "forgiving" when applying such plugins to workspace (e.g. a eclipse IDE plugin in the devfile shouldn't make it impossible to use in Che).

@l0rd
Copy link
Contributor Author

l0rd commented Mar 10, 2020

Discussed and agreed to go with "Current proposal"

@l0rd l0rd added this to the 2.0.0 milestone Jun 16, 2020
@davidfestal
Copy link
Collaborator

Implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Enhancement or issue related to the api/devfile specification kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants