WIP Make plugin references without version use 'latest' implicitly #13414

amisevsk · 2019-05-24T03:45:41Z

What does this PR do?

related issue - #12937
Changes necessary to make plugin references without a version specified,
e.g. eclipse/che-theia to parse as eclipse/che-theia/latest by default. As a side effect,
this commit disables:

Validation of plugin references with a custom registry. This allows
using direct links to meta.yamls (no need to have a path ending in
publisher/name/version) but also works by arbitrarily splitting the URL
into a 'registry' and 'id'
Devfile aliases and similar features are disabled for plugins from
custom registries
Some tests that depend on having a custom registry specified are
disabled until issue Improve/fix parsing of plugins/editors attribute #13385 is resolved.

See also: #13385, since I believe that the way we handle plugin references is in sore need of improvement.

This plugin is still WIP but I would like comments/review, especially regarding how custom registries are handled.

Changes necessary to make plugin references without a version specified, e.g. eclipse/che-theia to parse as 'eclipse/che-theia/latest' by default. As a side effect, this commit disables: - Validation of plugin references with a custom registry. This allows using direct links to meta.yamls (no need to have a path ending in publisher/name/version) but also works by arbitrarily splitting the URL into a 'registry' and 'id' - Devfile aliases and similar features are disabled for plugins from custom registries - Some tests that depend on having a custom registry specified are disabled until issue eclipse-che#13385 is resolved. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

mshaposhnik · 2019-05-24T06:50:44Z

IMO it is highly conflicting with #13297 There will be no possibilty to specify registry directly in ID, it will be in separate field.

ibuziuk · 2019-05-24T10:17:47Z

@mshaposhnik could you please clarify why it is highly conflicting with #13297 ? Do you mean impl details because in terms of functionality I do not see any conflict.

mshaposhnik · 2019-05-24T11:02:22Z

Both code & logic:

There will be no more registries in plugin ID. They're moved into separate field; So the parsing logic is changed;
Direct links to meta.yaml shouldn't be introduced here since they will be done in reference field in my PR etc
So for me i'ts better to limit scope of the PR just to default version and avoid those collisions with registry/references URL etc.

ibuziuk · 2019-05-24T11:07:31Z

@mshaposhnik how are you planning to proceed with #13414 ? are you going to merge it for CR1 ?

mshaposhnik · 2019-05-24T11:14:40Z

@ibuziuk we trying our best :)

ibuziuk · 2019-05-24T11:33:35Z

@mshaposhnik no sorry I meant your PR #13297 ? is it planned to be merged fairly soon?

for me i'ts better to limit scope of the PR just to default version and avoid those collistions with registry/refercences URL etc.

I agree, so wondering when are you planning to merge the fix for #13104 and we could adjust the latest version support of it @amisevsk wdyt ?

mshaposhnik · 2019-05-24T11:50:25Z

@ibuziuk yes i understood, we're solving last couple of problems (like setting memory limits and checking for default editors/plugins for those specified via reference) and will be ready to merge.

amisevsk · 2019-05-24T14:03:21Z

@mshaposhnik I don't think the two PRs conflict on a logic level, in fact I think your PR complements this PR pretty well in the general case. The main purpose of this PR is to allow plugin IDs without version specified (in which case they're rewritten with latest). The other changes present here are to allow that to work in don't-break-everything way. Basically, the whole "don't parse URLs" thing is a side effect and workaround.

PR #13297 simplifies this quite a bit actually, since it changes how plugin strings are written -- it would be easy to adapt this onto your open PR and this PR would probably become simpler.

I'm fine with waiting for #13297 to be merged and reworking this one to fit in with it.

That said, I'd appreciate comments on #13385, since I think using simple strings for plugins is becoming more and more of a problem and we should strongly consider moving away from it.

sleshchenko

PR description says that it is still WIP, is it?

sleshchenko · 2019-05-27T12:48:27Z

...e/src/test/java/org/eclipse/che/api/devfile/server/validator/DevfileSchemaValidatorTest.java

      {
        "editor_plugin_component/devfile_editor_plugins_components_with_invalid_memory_limit.yaml",
        "(/components/0/memoryLimit):The value must be of string type, but actual type is integer."
      },
      {
        "editor_plugin_component/devfile_editor_component_with_multiple_colons_in_id.yaml",
-        "(/components/0/id):The string value must match the pattern \"^((https?://)[a-zA-Z0-9_\\-./]+/)?[a-z0-9_\\-.]+/[a-z0-9_\\-.]+/[a-z0-9_\\-.]+$\"."
+        "(/components/0/id):The string value must match the pattern \"^((https?://)[a-zA-Z0-9_\\-./]+/)?[a-z0-9_\\-.]+/[a-z0-9_\\-.]+(/[a-z0-9_\\-.]+)?$\"."


Since we use slash instead of colon for separating version from name, this test case looks outdated. Could you consider removing it and add a new test case with invalid URL instead?

amisevsk · 2019-05-27T13:01:27Z

@sleshchenko I still consider it WIP because I'd like some discussion about the disabled/modified tests, and also it conflicts with #13297, and so may need to be significantly reworked.

sleshchenko · 2019-05-27T13:07:29Z

@sleshchenko I still consider it WIP because I'd like some discussion about the disabled/modified tests, and also it conflicts with #13297, and so may need to be significantly reworked.

Then please consider adding WIP into PR title to avoid occasional merge of this PR.

amisevsk · 2019-05-27T17:11:41Z

Then please consider adding WIP into PR title to avoid occasional merge of this PR.

It would need an approval first, so I'm not too worried ;)

amisevsk · 2019-08-21T03:19:26Z

Closing PR as woefully out of date; this would need to be reworked with other plugin changes.

amisevsk added the kind/enhancement A feature request - must adhere to the feature request template. label May 24, 2019

amisevsk requested review from ibuziuk, mshaposhnik and sleshchenko May 24, 2019 03:45

amisevsk requested review from metlos and skabashnyuk as code owners May 24, 2019 03:45

l0rd mentioned this pull request May 26, 2019

Add Devfiles which are based on Che 7 Stacks eclipse-che/che-devfile-registry#3

Merged

sleshchenko reviewed May 27, 2019

View reviewed changes

amisevsk changed the title ~~Make plugin references without version use 'latest' implicitly~~ WIP Make plugin references without version use 'latest' implicitly May 27, 2019

l0rd mentioned this pull request Jun 4, 2019

Che 7 GA #12696

Closed

ibuziuk mentioned this pull request Jun 11, 2019

Che 7 stacks should use version "latest" of plugins #13426

Closed

che-bot added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 6, 2019

eclipse-che deleted a comment from che-bot Aug 6, 2019

amisevsk closed this Aug 21, 2019

amisevsk deleted the plugin-latest-support branch August 21, 2019 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Make plugin references without version use 'latest' implicitly #13414

WIP Make plugin references without version use 'latest' implicitly #13414

amisevsk commented May 24, 2019 •

edited by ibuziuk

Loading

mshaposhnik commented May 24, 2019 •

edited

Loading

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019 •

edited

Loading

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019

amisevsk commented May 24, 2019

sleshchenko left a comment

sleshchenko May 27, 2019

amisevsk May 27, 2019

amisevsk commented May 27, 2019

sleshchenko commented May 27, 2019

amisevsk commented May 27, 2019

amisevsk commented Aug 21, 2019

WIP Make plugin references without version use 'latest' implicitly #13414

WIP Make plugin references without version use 'latest' implicitly #13414

Conversation

amisevsk commented May 24, 2019 • edited by ibuziuk Loading

What does this PR do?

mshaposhnik commented May 24, 2019 • edited Loading

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019 • edited Loading

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019

ibuziuk commented May 24, 2019

mshaposhnik commented May 24, 2019

amisevsk commented May 24, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko May 27, 2019

Choose a reason for hiding this comment

amisevsk May 27, 2019

Choose a reason for hiding this comment

amisevsk commented May 27, 2019

sleshchenko commented May 27, 2019

amisevsk commented May 27, 2019

amisevsk commented Aug 21, 2019

amisevsk commented May 24, 2019 •

edited by ibuziuk

Loading

mshaposhnik commented May 24, 2019 •

edited

Loading

mshaposhnik commented May 24, 2019 •

edited

Loading