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

Rename id field of chePlugin/cheEditor component in Devfile #13062

Closed
sleshchenko opened this issue Apr 3, 2019 · 10 comments
Closed

Rename id field of chePlugin/cheEditor component in Devfile #13062

sleshchenko opened this issue Apr 3, 2019 · 10 comments
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. severity/P1 Has a major impact to usage or development of the system.

Comments

@sleshchenko
Copy link
Member

sleshchenko commented Apr 3, 2019

Description

Currently, we have two possible formats for model of chePlugin/cheEditor component. First one is {PLUGIN_ID}:{PLUGIN_VERSION}, like the following

  name: theia
  type: cheEditor
  id: org.eclipse.che-theia:v1.0.0

Or another possible way how id may be defined with Custom Plugin Registry: {PLUGIN_REGISTRY_UR}/{PLUGIN_ID}:{PLUGIN_VERSION}, like the following:

  name: theia
  type: cheEditor
  id: https://my-plugin-registry.com/plugins/org.eclipse.che-theia:v1.0.0

And it's confusing that ID field is not identifier at all.
The goal of this issue is to find a better name for id field and rename it.
The initial proposal is to use reference instead of id.
Then we will have the following

  name: theia
  type: cheEditor
  reference: org.eclipse.che-theia:v1.0.0
  name: theia
  type: cheEditor
  reference: https://my-plugin-registry.com/plugins/org.eclipse.che-theia:v1.0.0

It looks more logical.
Note that kuberentes/openshift component has reference field too but with different format, it is {LOCAL_FILE_PATH} or {URL_TO_FILE_CONTENT} for them.

Note that in plugin registry naming format changes are coming and format of reference will be changed to something like <publisher>/<id>/<version>. The following issue eclipse-che/che-plugin-registry#55. Field value format can be handled in scope this issue or in a separate one.

@sleshchenko sleshchenko added the kind/task Internal things, technical debt, and to-do tasks to be performed. label Apr 3, 2019
@sleshchenko
Copy link
Member Author

cc @l0rd @slemeur @ibuziuk @garagatyi @skabashnyuk @mshaposhnik
Feel free to leave your opinion/alternative proposal as a comment or just react 👍 if you agree with proposed field renaming

@l0rd
Copy link
Contributor

l0rd commented Apr 3, 2019

@sleshchenko I am not sure.

An id is a name that identifies something in a unique way. A reference is a link to something. In the case of che-plugin it's a link to the meta.yaml you want to refer too?

In the simplest use case, when a user writes a devfile, he wants a simple way to define:

  • the project he wants to clone
  • the plugins he wants to add to his workspace

A project will be identified by a location and an optional branch (refspec to be more precise...)
A plugin will be identified by an name (org/plugin), an optional version and an optional repository-url.

Wouldn't be simpler to describe it like this:

  type: cheEditor
  name: eclipse/che-theia
  version: v1.0.0                                        # optional
  repository-url: https://my-plugin-registry.com/plugins # optional 

@skabashnyuk skabashnyuk added severity/P1 Has a major impact to usage or development of the system. team/platform labels Apr 4, 2019
@sleshchenko
Copy link
Member Author

In the case of che-plugin it's a link to the meta.yaml you want to refer too?

I know that guys from ide2 team use meta.yaml that is stored on gist which is not plugin registry at all. And then it could look like the following

  type: cheEditor
  reference: https://gist.githubusercontent.com/sleshchenko/a3b3034e0fa0220/raw/3029/meta.yaml

Actually, now they have to make gist to pretend plugin registry, and they configure id as https://raw.githubusercontent.com/eclipse/che-plugin-registry/upgrade-che-theia-dev/plugins/org.eclipse.che.theia.dev:0.0.1 when actual URL is https://raw.githubusercontent.com/eclipse/che-plugin-registry/upgrade-che-theia-dev/plugins/org.eclipse.che.theia.dev/0.0.1/meta.yaml.

For me, the following example looks logical

  type: cheEditor
  name: eclipse/che-theia
  reference: org.eclipse.che-theia:v1.0.0

because I understand it as a reference relative to Plugin Registry configured on Che Server. A reference here could be without version here as well.

I agree that multiple fields may be more straightforward than the single field(reference) with multiple formats. But multiple fields are less flexible, I mean if we need to add one more ability, like URL link to meta.yaml content - then we probably would need to introduce a new field to Component model. With a single reference field we keep our model as is.

I do not have a strong opinion whether we should use single field or multiple fields and below are listed my thoughts.
I think that URL to content of meta.yaml is a valid case that can be useful and actually is used now. So, we should consider it to support.

@sleshchenko
Copy link
Member Author

Note that in case of multiple fields, id is not enough since new naming convention[1] declares that id is unique per publisher and then it would look like the following:

  type: cheEditor
  name: theia-editor
  publisher: redhat
  id: eclipse/che-theia
  version: v1.0.0                                        # optional
  repository-url: https://my-plugin-registry.com/plugins # optional 

Or id should include publisher what is not obvious thing to do

  type: cheEditor
  name: theia-editor
  id: redhat/eclipse/che-theia
  version: v1.0.0                                        # optional
  repository-url: https://my-plugin-registry.com/plugins # optional 

[1] eclipse-che/che-plugin-registry#55

@l0rd
Copy link
Contributor

l0rd commented Apr 4, 2019

I think there was a misunderstanding. In my proposal:

type: cheEditor
name: eclipse/che-theia
version: v1.0.0                                        # optional
repository-url: https://my-plugin-registry.com/plugins # optional
  • the name field has already the publisher. In fact the format is / (i.e. eclipse in the sample is the publisher). We can split it in two distinct fields (publisher and name but the name without the publisher doesn't make sense)
  • there is no id field anymore . That's because id and name looks redundant and the plugin is identified name + version
  • what you call reference is actually 'repository-url/name/version` and the case of a meta.yaml in a gist is still supported

@sleshchenko
Copy link
Member Author

Sorry, maybe I missed something in eclipse-che/che-plugin-registry#55
Is there any plan to change format of id and name fields in meta.yaml? Because as far as I understand id should be used for identifying a plugin, and name is just a pretty name ( the same as diplayName in OpenShift)
Here is existing plugin where a name is just display name but not Identifier
https://github.com/eclipse/che-plugin-registry/blob/6a90b61f6aa713767f5c137d9fb55aba070a6cf1/plugins/ms-vscode.node-debug/1.32.1/meta.yaml#L1
And here is Alex sample with new meta.yaml where name looks like displayName but not identifier eclipse-che/che-plugin-registry#55 (comment)

Also, I think that it may be confusing to put publisher and name of plugin into component name field, because a component name field for other component types is just alias that may be used for referencing them by commands. So, a component name used to have a free value that the user wants, and user do not have to duplicate publisher/pluginName in all commands that references this component, for example(current format is used)

specVersion: 0.0.1
name: test
components:
  - type: cheEditor
    name: theia
    id: org.eclipse.che-theia:next 
commands:
  - name: hello
    actions:
      type:exec
      component: theia
      command: echo Hello
  - name: currDir
    actions:
      type:exec
      component: theia
      command: pwd

Then if a user wants to use theia but defined as another editorId, or even own dockerimage component, we just modify the component, but not commands that reference it.

@l0rd Does it make sense?

@l0rd
Copy link
Contributor

l0rd commented Apr 4, 2019

Good points @sleshchenko

You have perfectly summarize the problem with the name field. We use it in both meta.yaml and devfile but they do not have the same meaning: one is a display name and the other is an alias. Why don't we call them like that (display name and alias)? Wouldn't it be clearer?

For the id field: we can keep id and publisher as 2 separate fields as in the meta.yaml. That would be consistent and I understand that. But keep in mind that

  • devfile consumers and authors are usually not plugin developers (i.e. they have never seen a meta.yaml in their life)
  • A devfile should be short and easy to read
  • An id field without the publisher doesn't make sense in a devfile. They are highly coupled: if I add the id field and forget to add a publisher field my workspace won't load. With 2 fields users have more chances to make mistakes.

So do we really want to sacrifice the user UX for the sake of consistency between devfile and meta.yaml?

@sleshchenko
Copy link
Member Author

@l0rd

Why don't we call them like that (display name and alias)? Wouldn't it be clearer?

Let me clarify, you propose rename name in meta.yaml to displayName and
name in a component model to alias, right? If yes, I like this idea.
What about id field in meta.yaml? Should it be renamed to name?

So do we really want to sacrifice the user UX for the sake of consistency between devfile and meta.yaml?

Let me summarize what I've written.
I do not want to keep consistent devfile and meta.yaml, and I think that reference field with multiple formats is good enough.
But I'm OK with multiple fields (registryUrl, name, version) but I see the following issues here:

  1. How user will be able to define plugin/editor that which meta.yaml content is accessible via HTTP but it is not published to plugin registry yet?
  2. When user writes Devfile, where he supposed to get id/name of plugin? In plugin registry? Like https://che-plugin-registry.openshift.io/plugins? If yes, then the format of single field name with format publisher/name would be not obvious.
  3. Repository URL is https://che-plugin-registry.openshift.io/plugins, but why repository URL should include plugins path segment? I guess it is question to Plugin Registry, and maybe https://che-plugin-registry.openshift.io/ should be a root of plugin registry that responds with index.json instead of README.md

@amisevsk
Copy link
Contributor

amisevsk commented Apr 5, 2019

A PR I'm working on may be somewhat relevant here: #12998.

It changes plugin broker to work off of id, version, and registry and download plugins from registry/plugins/id/version/meta.yaml at the moment, and would likely need to be modified if we go a different direction here.

@sleshchenko
Copy link
Member Author

As a result of discussion that took place here the following changes are comming:

  1. Introduce registryUrl field for cheEditor/chePlugin components Introduce registryUrl field for cheEditor/chePlugin components #13104
  2. Rework a bit format of meta.yaml. It is listed here Naming convention che-plugin-registry#55 (comment)
    2.1 name should be renamed displayName
    2.2. id should be renamed name
    2.3. in the generated che plugin registry index.json we should add an id field that is the concatenation of publisher/name/version

So, the current issue is not actual anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

4 participants