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

Add possibility to specify reference or registry url for chePlugin/cheEditor type components #13297

Merged
merged 37 commits into from
Jun 10, 2019

Conversation

mshaposhnik
Copy link
Contributor

@mshaposhnik mshaposhnik commented May 8, 2019

This is a PR containing WS master part of adding possibility to define chePlugin/cheEditor type components using absolute reference to plugin meta or using custom registryUrl-s.

Example:

specVersion: 0.0.1
name: terminal-sample
components:
  - type: chePlugin
    reference: https://raw.githubusercontent.com/eclipse/che-plugin-registry/myplugin/meta.yaml
specVersion: 0.0.1
name: terminal-sample
components:
  - type: chePlugin
    registryUrl: https://raw.githubusercontent.com/eclipse/che-plugin-registry/master/
    id: publisher/terminal-sample/0.0.1

Fixes Introduce registryUrl field for cheEditor/chePlugin components #13104

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
if (!isNullOrEmpty(reference)) {
workspaceConfig.getAttributes().put(WORKSPACE_TOOLING_EDITOR_ATTRIBUTE, reference);
} else {
String compositeId = registryUrl != null ? registryUrl + "#" + editorId : editorId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make this a method of the PluginFQNParser or maybe rather PluginFQN? Could be combined with the logic of parsePluginFQN below and result in a simpler flow here.

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@sleshchenko
Copy link
Member

@mshaposhnik Please link it with the issue you're working on

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm becoming increasingly against our current format of using one raw string to specify 5-6 fields about a plugin (see #13414).

There will always be corner and edge cases we can't handle when we're trying to extract structure from a raw string (e.g. valid URLs with # in them) and these changes make the plugin reference spec even more arcane and difficult for users to understand.

If this PR already contains breaking changes, why not replace plugin references with json instead of strings?

{
  "registry": "https://mycustomregistry.optional",
  "id": "publisher/name/version",
  "reference": "optional direct link"
}

Is much easier to handle, and in the common case (reference to a plugin in the default registry) would simplify to what we currently have:

{"id": "publisher/name/version"}

I think we're focussed too much on sticking to non-ideal format just becuase it's what's already used.

@mshaposhnik
Copy link
Contributor Author

@amisevsk We discussed your proposal and all agreed that we definitely need to try with json, but in separate issue/PR. Before that, we will also create another issue to remove outdated devfile API which stores workspaces with old properties format into DB, to minimalize amount of broken WS after we migrate to the new format.

@l0rd l0rd mentioned this pull request May 28, 2019
@mshaposhnik
Copy link
Contributor Author

ci-build

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other looks good

Copy link
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I only have a couple of minor comments.

@mshaposhnik
Copy link
Contributor Author

ci-test

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job 👍
This PR brings a really useful feature!

@mshaposhnik
Copy link
Contributor Author

ci-build

@che-bot
Copy link
Contributor

che-bot commented Jun 10, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13297
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@artaleks9
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1844/) doesn't show any regression against this Pull Request.

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 this pull request may close these issues.

None yet

7 participants