-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
if (!isNullOrEmpty(reference)) { | ||
workspaceConfig.getAttributes().put(WORKSPACE_TOOLING_EDITOR_ATTRIBUTE, reference); | ||
} else { | ||
String compositeId = registryUrl != null ? registryUrl + "#" + editorId : editorId; |
There was a problem hiding this comment.
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 <[email protected]>
...core-api-model/src/main/java/org/eclipse/che/api/core/model/workspace/devfile/Component.java
Outdated
Show resolved
Hide resolved
...vfile/src/main/java/org/eclipse/che/api/devfile/server/convert/DefaultEditorProvisioner.java
Outdated
Show resolved
Hide resolved
...vfile/src/main/java/org/eclipse/che/api/devfile/server/convert/DefaultEditorProvisioner.java
Outdated
Show resolved
Hide resolved
wsmaster/che-core-api-devfile/src/main/resources/schema/devfile.json
Outdated
Show resolved
Hide resolved
...e/src/test/java/org/eclipse/che/api/devfile/server/validator/DevfileSchemaValidatorTest.java
Outdated
Show resolved
Hide resolved
...rces/schema_test/editor_plugin_component/devfile_editor_component_with_id_and_reference.yaml
Outdated
Show resolved
Hide resolved
...t/resources/schema_test/editor_plugin_component/devfile_editor_component_with_reference.yaml
Outdated
Show resolved
Hide resolved
...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java
Show resolved
Hide resolved
@mshaposhnik Please link it with the issue you're working on |
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
Signed-off-by: Max Shaposhnik <[email protected]>
There was a problem hiding this 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.
@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. |
ci-build |
...ema/src/main/resources/che-schema/7.0.0-beta6.0/2__add_registry_url_to_devfile_component.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other looks good
There was a problem hiding this 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.
...ace/src/main/java/org/eclipse/che/api/workspace/server/devfile/convert/DevfileConverter.java
Outdated
Show resolved
Hide resolved
...api/workspace/server/devfile/convert/component/plugin/PluginComponentToWorkspaceApplier.java
Outdated
Show resolved
Hide resolved
.../java/org/eclipse/che/api/workspace/server/devfile/convert/DefaultEditorProvisionerTest.java
Outdated
Show resolved
Hide resolved
...pace-shared/src/main/java/org/eclipse/che/api/workspace/shared/dto/devfile/ComponentDto.java
Outdated
Show resolved
Hide resolved
...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java
Outdated
Show resolved
Hide resolved
...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java
Outdated
Show resolved
Hide resolved
wsmaster/che-core-api-workspace/src/main/resources/schema/devfile.json
Outdated
Show resolved
Hide resolved
...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java
Outdated
Show resolved
Hide resolved
wsmaster/che-core-api-workspace/src/main/resources/schema/devfile.json
Outdated
Show resolved
Hide resolved
ci-test |
wsmaster/che-core-api-workspace/src/main/resources/schema/devfile.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
ci-build |
Results of automated E2E tests of Eclipse Che Multiuser on OCP: |
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. |
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:
Fixes Introduce registryUrl field for cheEditor/chePlugin components #13104