search

eclipse-che / che-plugin-registry

Eclipse Public License 2.0
41 stars 151 forks source link

Naming convention #55

Closed l0rd closed 5 years ago

l0rd commented 6 years ago

There is no proper naming convention for Che plugins yet. This is issue is to propose one. The goal is to make something simple but still powerful, more similar to the VS Code extensions naming convention than Java packages.

tsmaeder commented 6 years ago
  1. How does a person or organization get a publisher-id?
  2. Can you please give a concrete example of publisher-id?
benoitf commented 6 years ago

We discussed as well to remove some stuff from yaml file (id, version) as it's duplicating the directory layout, should it be part of this enhancement or another one ?

benoitf commented 6 years ago

@tsmaeder

  1. as it's based on folders, if folder exists in https://github.com/eclipse/che-plugin-registry/tree/master/plugins then it is taken
  2. example of publisher id : redhat
tsmaeder commented 6 years ago 
1. as it's based on folders, if folder exists in https://github.com/eclipse/che-plugin-registry/tree/master/plugins then it is `taken`

So a committer decides whether to give a publisher-id to a certain when he accepts a PR with some folder name (like 'redhat')? What about squatters? How do we ensure that only redhatters can make 'redhat' plugins?

benoitf commented 6 years ago

I would say it's up to reviewers. Also in meta.yaml you're referencing a repository. So probably if you try to propose redhat/cool-plugin but it ends to a repository http://github.com/thecoolguy/ then you can probably think that it's not part of a Red Hat project.

I would say that this kind of features (Creating an account, publishing on its own namespace is for marketplace, here registry is just exposing the directory layout)

l0rd commented 6 years ago

@tsmaeder this should be a discussion about a naming convention, not about how to fight squatters. I mean if the name of a plugin is com.redhat.developers.che.helloworld or redhat-developers/che-helloworld we should still fight squatters.

garagatyi commented 5 years ago

After discussing with @l0rd and @ibuziuk we decided to move files in Che plugin registry according to the suggested FS structure. We also want to add model versioning to the structure, so it would be possible to change layout of meta files model in registry just by changing model version part in the path to the meta.yaml. For example: Now: plugin_ID/version/ After this issue: /v1/plugins/publisher_id/plugin_id/version/ /v2/plugins/publisher_id/plugin_id/version/ /v3/plugins/publisher_id/plugin_id/version/ This would require checking that known components that uses plugin registry work well with the change.

Since previous Che deployments use plugin registry deployed at Openshift.io we can also add plugins folder without version prefix with current meta.yaml files and keep it for several versions until we are OK with incompatible change. @l0rd @ibuziuk @davidfestal @amisevsk WDYT?

garagatyi commented 5 years ago

BTW if user created workspace with registry that uses current notation and then we update registry on Openshift.io to a new scheme user's workspaces would not start. Am I missing something?

garagatyi commented 5 years ago

@l0rd who should be the publisher when Red Hat employee created Che plugin entry with VS Code extension authored by Microsoft on the VS Code marketplace

benoitf commented 5 years ago

I don't know if it's related to the structure but most of VS Code extensions published on the plug-in registry have the theia-endpoint:next as endpoint so they basically work only with che-theia:next

Is there a way to ensure that when we run che-theia:latest we don't see :next plugins ? (like a requirement)

or we need to think how to provide the theia endpoint without adding it in the image.

l0rd commented 5 years ago

@l0rd who should be the publisher when Red Hat employee created Che plugin entry with VS Code extension authored by Microsoft on the VS Code marketplace

@garagatyi I would say that for a VS Code extension publisher and name in the che plugin registry should match the ones of the marketplace. Even if an employee from another company has added it to the registry

@benoitf I think the proper way is to provide the theia endpoint without adding it in the image.

l0rd commented 5 years ago

Otherwise @garagatyi I am ok with the description of the implementation

amisevsk commented 5 years ago

Me too, makes sense to me.

garagatyi commented 5 years ago

@benoitf we could use proper versioning to declare this dependency on the Theia version. Though we don't have it in place ATM

benoitf commented 5 years ago

@garagatyi I kind of like the idea to move out the endpoint runtime from the docker image (so the image contains only the plug-in + the runtime of this plug-in but is not tied to the endpoint (so no need to inherit from custom images))

garagatyi commented 5 years ago

@benoitf do you mean that image should include proper version of Node.JS or you are thinking about injecting it with the plugin runner?

benoitf commented 5 years ago

@garagatyi injecting it (I experimented it using nexe or pkg to make a single executable). I had to recompile nodejs for alpine to avoid the requirement on libstdc++ https://github.com/zeit/pkg/issues/555). But basically we could have one binary for alpine and one binary for linux/amd64 (I don't have checked how to run it but it could be through a PV, copy or any other great idea :)

garagatyi commented 5 years ago

We need a plan how to migrate existing workspaces from old notation to a new one. Just versioning won't help us since users will stuck with old non-evolving plugins and changing registry prefix to v2 will break such workspaces. We could leave existing plugins on their places and add plugins with the new notation. This would avoid breaking old workspaces after registry update. Then we need some way to deprecate these plugins and make users migrate to new ones. We can handle that with the help of user dashboard:

garagatyi commented 5 years ago

@davidfestal @ibuziuk @amisevsk It is topic we discussed today on the standup call. Feel free to provide your ideas

garagatyi commented 5 years ago

@l0rd you didn't mention what should be the separator between publisher_id and plugin_id when we use it in plugins list in workspace config. I assume that version is separated by colon :. Did you mean attributes look like:

{
  "attributes": {
      "editor": "redhat.theia-ide:2.3.4",
      "plugin": "microsoft.typescript-ls:1.0.11"
  }
}

or maybe 

{
  "attributes": {
      "editor": "redhat/theia-ide:2.3.4",
      "plugin": "microsoft/typescript-ls:1.0.11"
  }
}

or maybe we should change model and put publisher id, plugin id and version in different fields, e.g.

{
  "attributes": {
  },
  "editor": {
      "publisher": "redhat",
      "id": "theia-ide",
      "version": "2.3.4"
  },
  "plugins": [ 
      {
          "publisher": "microsoft",
          "id": "typescript-ls",
          "version": "1.0.11"
      },
      {
          "publisher": "redhat",
          "id": "openshift-connector",
          "version": "4.0.8"
      }
  ]
}
l0rd commented 5 years ago

@garagatyi your second proposal makes more sense to me. What do you think?

garagatyi commented 5 years ago

@l0rd yes, second option is more aligned with idea of publisher == github organization in the issue description.

garagatyi commented 5 years ago

Here is the flow I have in mind:

  1. Cases we need to cover: 1.1. Old che with old plugins in ws 1.2. New che with old plugins in ws 1.3. New che with new plugins in ws 1.4. Custom registry and hosting meta on GH 1.5. Factories with old plugins 1.6. Devfiles hosted in some repos with old plugins
  2. Do not introduce v1,v2 prefixes since in our case we change location of files and using prefixes would not help much here. For model versioning apiVersion should work better cause it doesn’t require retrying download of meta from another location, especially when several version passed and user still used very old version and several retries would be needed.
  3. New flow on master/broker 3.1. If old format of attributes (org.eclipse.che.theia:1.0.0) is used download from old location /plugins/<plugin_id>/<plugin_version>/meta.yaml, otherwise download from the new one /plugins/<publisher_id>/<plugin_id>/<pluign_version>/meta.yaml 3.2. Add deprecation section to meta.yaml model. See format below. 3.3. When significant amount of time has passed after the deprecation old plugin can be removed 3.4. Make broker produce warning in broker output (shown on workspace start view) and added to workspace runtime if plugin with deprecation.deprecated:true is used 3.5. If deprecation.autoMigrate:true broker uses new plugin specified in deprecation.migrateTo instead of old automatically
  4. Changes needed: 4.1. In registry restructure files, update CI scripts (including because old Ids are incompatible with new ones), update validation, add publisher to metas, add deprecation info into index file. 4.2. In Che master | broker: support downloading from new AND old location. Add publisher to meta. Change validation of IDs 4.3. In UD support new notation with publisher and / between publisher and id. Keep support of old notation too. 4.4. Ideally add features for UD:
    • to show warning when plugin is deprecated (we can include this info in index of registry to ease this feature)

Deprecation section:

deprecation:
  deprecated: true
  migrateTo: "redhat-developers/theia:1.2.3"
  autoMigrate: true

Old meta.yaml:

publisher: <empty for old model metas>
id: ms-python.python
version: 2019.2.5433
deprecation:
  deprecated: true
  autoMigrate: true
  migrateTo: ms-python/python:2019.2.5433
type: VS Code extension
name: Python
title: Python extension
description: Linting, Debugging (multi-threaded, remote), Intellisense, code formatting, refactoring, unit tests, snippets, and more.
icon: https://www.eclipse.org/che/images/logo-eclipseche.svg
url: https://marketplace.visualstudio.com/_apis/public/gallery/publishers/ms-python/vsextensions/python/2019.2.5433/vspackage
publisher: Red Hat, Inc.
repository: https://github.com/Microsoft/vscode-python
category: Language
firstPublicationDate: "2019-03-05"
attributes:
  containerImage: "eclipse/che-remote-plugin-python-3.7.2:next"

New meta.yaml:

publisher: ms-python
id: python
version: 2019.2.5433
deprecation: # is empty or deprecation.deprecated is false
type: VS Code extension
name: Python
title: Python extension
description: Linting, Debugging (multi-threaded, remote), Intellisense, code formatting, refactoring, unit tests, snippets, and more.
icon: https://www.eclipse.org/che/images/logo-eclipseche.svg
url: https://marketplace.visualstudio.com/_apis/public/gallery/publishers/ms-python/vsextensions/python/2019.2.5433/vspackage
publisher: Red Hat, Inc.
repository: https://github.com/Microsoft/vscode-python
category: Language
firstPublicationDate: "2019-03-05"
attributes:
  containerImage: "eclipse/che-remote-plugin-python-3.7.2:next"

Additional notes:

@l0rd @ibuziuk @davidfestal @amisevsk @skabashnyuk WDYT?

ibuziuk commented 5 years ago

Do not introduce v1,v2 prefixes since in our case we change location of files and using prefixes would not help much here

@garagatyi don't you think that not having v1, v2 etc. makes migration mechanism less grokable?

For model versioning apiVersion should work better cause it doesn’t require retrying download of meta from another location

do you mean apiVersion in plugin's meta yaml?

deprecation: deprecated: true

IMO deprecated: true is redundant field - deprecation itself should be optional as I understand

garagatyi commented 5 years ago

do you mean apiVersion in plugin's meta yaml?

yes

IMO deprecated: true is redundant field - deprecation itself should be optional as I understand

Maybe we can use the following structure and avoid both deprecate:true and not often used word deprecation 

deprecated:
   migrateTo: ...
   autoMigrate: true

WDYT?

Do not introduce v1,v2 prefixes since in our case we change location of files and using prefixes would not help much here

@garagatyi don't you think that not having v1, v2 etc. makes migration mechanism less grokable?

Can you elaborate on how it would help?

ibuziuk commented 5 years ago

deprecated: migrateTo: ... autoMigrate: true

yeah, looks better IMO

don't you think that not having v1, v2 etc. makes migration mechanism less grokable? Can you elaborate on how it would help?

no strong opinion on that. I believe if apiVersion is defined in meta yaml no need to have v1 / v2 at all which is more like REST versioning approach

garagatyi commented 5 years ago

Additional details for the assignee: 1.Change UD to support old registry/plugin_id:version and new registry/publisher_id/plugin_id:version notations. From what I know we only need to change https://github.com/eclipse/che/blob/432e3ac4414e0a70bb3dc51fbe33ebfd5b665a62/dashboard/src/components/api/plugin-registry.factory.ts#L15 and https://github.com/eclipse/che/blob/e689a3291aeb467c30ed2164303353231a5328ca/dashboard/src/app/workspaces/workspace-details/workspace-plugins/workspace-plugins.controller.ts

  1. Change WS master to support old and new notation in plugins FQNs in workspace attributes. Check that devfile is handling that too. Here is an approximate solution https://github.com/eclipse/che/compare/master...garagatyi:publisher?expand=1#diff-e19a15858ced9af7855c60489eccc6adR174
  2. Change unified plugin broker to respect old and new notations. Angel's PR tackles old notation. We will just need to add publisher field to meta FQN model and add publisher/ between registry and plugin ID when constructing URL to download meta.yaml. If publisher is missing do not add it - it would add backward compatibility with old plugins after changes in registry.
  3. Changes in registry might be the most significant ones. We need to:
    • copy meta.yamls from plugins/plugin_id/version/meta.yaml to plugins/publisher_id/plugin_id/version/meta.yaml
    • add publisher field to meta.yaml files placed at plugins/publisher_id/plugin_id/version/meta.yaml. For VS Code extension based plugins set VS Code marketplace extension publisher and ID. Sync with Mario to set correct publisher to other plugins. Old meta.yamls should have empty publisher.
    • add deprecate section into each plugin that is staying at the old (current) place 
      publisher: ""
id: old_plugin_id
version: 1.2.3
name: Some plugin
deprecate:
autoMigrate: true
migrateTo: publisher_id/new_plugin_id:1.2.3
    • adapt CI scripts to respect both old and new notation
  4. In RhChe fix editor prefetching functionality. Ask @davidfestal for details
l0rd commented 5 years ago

Discussing with @sleshchenko about this issue and its consequences on https://github.com/eclipse/che/issues/13062 we agreed that in the new meta.yaml:

In the devfile we will use id and the an optional registryURL to specify a plugin to be added to a workspace.

garagatyi commented 5 years ago

version: in the form MAJOR.MINOR.PATCH and using the Semantic Versioning

@l0rd you are suggesting using semiver for version but we currently use terms next and master in Theia editor. Should we rename existing editors/plugins versions and possibly brake OSIO-Che or should we allow versions other than semiver compliant?

garagatyi commented 5 years ago

@benoitf FYI

l0rd commented 5 years ago

There was a che-dev thread about that. Considered the latest developments and the experience we have today I think we should not stick with semVer anymore. I have suggested -.

garagatyi commented 5 years ago

Oh, I missed that

sleshchenko commented 5 years ago

in the generated che plugin registry index.json we should add an id field that is the concatenation of publisher/name/version

After the latest agreement in cheEditor/chePlugin format id as publisher/name/version is not required anymore. We can continue using : as a separator for name and version -> publisher/name:version. Personally, I prefer this one since it's more common style and for example, docker uses it for images versioning as well. @l0rd @garagatyi Which style do you prefer?

garagatyi commented 5 years ago

@sleshchenko I'm OK with either

garagatyi commented 5 years ago

@sleshchenko can you elaborate on why you and Mario decided to include id in registry index as publisher/name/version instead of just publisher/name?

sleshchenko commented 5 years ago

To make user able to copy/paste id from the registry index and use it for his Devfile. Also, a combination of publisher and name is not identical, and publisher/name/version looks like a thing that identifies the plugin. @garagatyi Does it make sense?

garagatyi commented 5 years ago

@sleshchenko yes, thanks for the information. Interesting part here is that if we include version in a plugin ID and then update plugin registry on OSIO, UD in current Che installations would add :version to it. In WS config it would look like:

garagatyi commented 5 years ago

@ibuziuk @l0rd should we ensure that deploying new version of the registry on OSIO won't break Che installations of previous version?

l0rd commented 5 years ago

@sleshchenko I prefer the slash / rather than the semicolon : because then id will match the path of the meta.yaml URL. And a complete URL (gist or pastebin) could be composed with registryUrl + id. That was the rationale behind that. But now we have the reference as well so if we need to specify a meta.yaml URL a user should use that instead. So I am ok with publisher/name:version as well.

Something that we should keep in mind is that we need to implement eclipse/che/issues/12937 hence I would expect that 99% of the time users won't specify the version of the plugin in their devfile (they would just want latest).

@garagatyi No we do not want to break previous Che versions, that's why we should add the plugins with the new naming to the v2 folder.

garagatyi commented 5 years ago

Just discussed with @sleshchenko plugins upgrade policies taking into account new notation both with colon eclipse/che-theia:^0.1.0, eclipse/che-theia:~0.1.0 and with slash eclipse/che-theia/^0.1.0, eclipse/che-theia/~0.1.0 tilde and caret look a bit weird. Maybe colon is not that weird. But usage of a separate field would look better:

tool:
  id: eclipse/che-theia
  registryUrl: https://redhotchili.peppers/che-registry/
  version: ~0.1.0
garagatyi commented 5 years ago

But again, as Mario said id in this case is not really a unique identifier, but rather name of the plugin

l0rd commented 5 years ago

@garagatyi @sleshchenko I know you guys like to find new challenges but we are not going to support those ~ and ^ scenarios in a devfile. Please have a look at About versions of Che 7 plugins and editors che-dev thread were we discuss about that.

Nice registryUrl name by the way :guitar:

garagatyi commented 5 years ago

@l0rd right, sorry, I forgot about that agreement.

garagatyi commented 5 years ago

I'm going to use slash as version separator in new naming convention as Mario like it more and Sergii and I hesitating to choose

ibuziuk commented 5 years ago

@garagatyi am I correct that once https://github.com/eclipse/che/pull/13204 is merged we can close the issue?

garagatyi commented 5 years ago

@ibuziuk there are 2 other PRs to adapt chectl and rh-che to the naming convention changes https://github.com/che-incubator/chectl/pull/89, https://github.com/redhat-developer/rh-che/pull/1390 We could track them as separate issues, but they are related to this issue, actually.

ibuziuk commented 5 years ago

let's close it once the pending PRs are merged: