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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions wsmaster/che-core-api-devfile/src/main/resources/schema/devfile.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@
"alias": {},
"id": {
"type": "string",
"description": "Describes the component id. It has the following format: [{REGISTRY_URL}/]{plugin/editor PUBLISHER}/{plugin/editor NAME}/{plugin/editor VERSION}, where {REGISTRY_URL}/ is an optional part.",
"pattern": "^((https?://)[a-zA-Z0-9_\\-./]+/)?[a-z0-9_\\-.]+/[a-z0-9_\\-.]+/[a-z0-9_\\-.]+$",
"description": "Describes the component id. It has the following format: [{REGISTRY_URL}/]{plugin/editor PUBLISHER}/{plugin/editor NAME}/{plugin/editor VERSION}, where {REGISTRY_URL}/ is an optional part. If /{VERSION} is omitted, 'latest' is implicitly applied.",
"pattern": "^((https?://)[a-zA-Z0-9_\\-./]+/)?[a-z0-9_\\-.]+/[a-z0-9_\\-.]+(/[a-z0-9_\\-.]+)?$",
"examples": [
"eclipse/maven-jdk8/1.0.0",
"https://che-plugin-registry.openshift.io/eclipse/maven-jdk8/1.0.0"
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/org/eclipse/che/api/devfile/server/convert/DefaultEditorProvisionerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void shouldProvisionDefaultPluginsIfTheyAreNotSpecifiedAndDefaultEditorIs
assertTrue(components.contains(new ComponentImpl(PLUGIN_COMPONENT_TYPE, TERMINAL_PLUGIN_REF)));
}

@Test
@Test(enabled = false) // Issue: https://github.com/eclipse/che/issues/13385
public void
shouldProvisionDefaultPluginsIfTheyAreNotSpecifiedAndDefaultEditorFromCustomRegistryIsConfigured()
throws DevfileException {
Expand Down
2 changes: 1 addition & 1 deletion ...he/api/devfile/server/convert/component/editor/EditorComponentToWorkspaceApplierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public void shouldProvisionPluginCommandAttributesDuringCheEditorComponentApplyi
"eclipse/super-editor/0.0.1");
}

@Test
@Test(enabled = false) // Issue: https://github.com/eclipse/che/issues/13385
public void shouldProvisionPluginCommandAttributeWhenIdIsURLToCustomPluginRegistry()
throws DevfileException {
// given
Expand Down
2 changes: 1 addition & 1 deletion ...he/api/devfile/server/convert/component/plugin/PluginComponentToWorkspaceApplierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void shouldProvisionPluginCommandAttributesDuringChePluginComponentApplyi
"eclipse/super-plugin/0.0.1");
}

@Test
@Test(enabled = false) // Issue: https://github.com/eclipse/che/issues/13385
public void shouldProvisionPluginCommandAttributeWhenIdIsURLToCustomPluginRegistry()
throws DevfileException {
// given
Expand Down
2 changes: 1 addition & 1 deletion ...va/org/eclipse/che/api/devfile/server/convert/component/plugin/PluginProvisionerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public void shouldProvisionChePluginComponent() throws WorkspaceExportException
.getAttributes()
.put(
WORKSPACE_TOOLING_PLUGINS_ATTRIBUTE,
"eclipse/super-plugin/0.0.1,https://localhost:8080/publisher1/custom-plugin/v1");
"eclipse/super-plugin/0.0.1,publisher1/custom-plugin/v1");
workspaceConfig
.getAttributes()
.put(
Expand Down
9 changes: 3 additions & 6 deletions ...rc/test/java/org/eclipse/che/api/devfile/server/validator/DevfileSchemaValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ public Object[][] validDevfiles() {
{"dockerimage_component/devfile_dockerimage_component.yaml"},
{"dockerimage_component/devfile_dockerimage_component_without_entry_point.yaml"},
{"editor_plugin_component/devfile_editor_component_with_custom_registry.yaml"},
{"editor_plugin_component/devfile_editor_plugins_components_with_memory_limit.yaml"}
{"editor_plugin_component/devfile_editor_plugins_components_with_memory_limit.yaml"},
{"editor_plugin_component/devfile_editor_component_without_version.yaml"},
};
}

Expand Down Expand Up @@ -134,17 +135,13 @@ public Object[][] invalidDevfiles() {
"editor_plugin_component/devfile_editor_component_with_indistinctive_field_reference.yaml",
"(/components/0/reference):The object must not have a property whose name is \"reference\"."
},
{
"editor_plugin_component/devfile_editor_component_without_version.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_\\-.]+$\"."
},
{
"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_\\-.]+)?$\"."
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

},
// kubernetes/openshift component model testing
{
Expand Down
59 changes: 38 additions & 21 deletions ...rkspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,11 @@
public class PluginFQNParser {

private static final String INCORRECT_PLUGIN_FORMAT_TEMPLATE =
"Plugin '%s' has incorrect format. Should be: 'registryURL/publisher/name/version' or 'publisher/name/version'";
private static final String REGISTRY_PATTERN = "https?://[-./\\w]+(:[0-9]+)?(/[-./\\w]+)?";
"Plugin '%s' has incorrect format. Should be: 'registryURL/path', 'publisher/name/version', or 'publisher/name'";

// First potential format: 'publisher/name/version', where 'version' is optional. No alternate
// registry
// is permitted here.
private static final String PUBLISHER_PATTERN = "[-a-z0-9]+";
private static final String NAME_PATTERN = "[-a-z0-9]+";
private static final String VERSION_PATTERN = "[-.a-z0-9]+";
Expand All @@ -52,11 +55,18 @@ public class PluginFQNParser {
+ PUBLISHER_PATTERN
+ ")/(?<name>"
+ NAME_PATTERN
+ ")/(?<version>"
+ ")(:?/(?<version>"
+ VERSION_PATTERN
+ ")";
private static final Pattern PLUGIN_PATTERN =
Pattern.compile("((?<registry>" + REGISTRY_PATTERN + ")/)?(?<id>" + ID_PATTERN + ")");
+ "))?";
private static final Pattern PLUGIN_PATTERN = Pattern.compile("(?<id>" + ID_PATTERN + ")");
private static final String LATEST_VERSION = "latest";

// Second potential format: 'registryURL/identifier'. We do not extract publisher, name, and
// version here.
private static final String REGISTRY_PATTERN = "https?://[-./\\w]+(:[0-9]+)?(/[-./\\w]+)?";
private static final String REGISTRY_ID_PATTERN = "[-_.A-Za-z0-9]+";
private static final Pattern PLUGIN_WITH_REGISTRY_PATTERN =
Pattern.compile("(?<registry>" + REGISTRY_PATTERN + "/)(?<id>" + REGISTRY_ID_PATTERN + ")/?");

/**
* Parses a workspace attributes map into a collection of {@link PluginFQN}.
Expand Down Expand Up @@ -115,23 +125,28 @@ private Collection<PluginFQN> parsePluginFQNs(String attribute) throws Infrastru
}

public ExtendedPluginFQN parsePluginFQN(String plugin) throws InfrastructureException {
String registry;
String id;
String publisher;
String name;
String version;
URI registryURI = null;
// Check if plugin matches default format: 'publisher/name/version' or 'publisher/name'
Matcher matcher = PLUGIN_PATTERN.matcher(plugin);
if (matcher.matches()) {
registry = matcher.group("registry");
id = matcher.group("id");
publisher = matcher.group("publisher");
name = matcher.group("name");
version = matcher.group("version");
} else {
throw new InfrastructureException(format(INCORRECT_PLUGIN_FORMAT_TEMPLATE, plugin));
String id = matcher.group("id");
String publisher = matcher.group("publisher");
String name = matcher.group("name");
String version = matcher.group("version");
if (isNullOrEmpty(version)) {
version = LATEST_VERSION;
id = String.format("%s/%s", id, version);
}
return new ExtendedPluginFQN(null, id, publisher, name, version);
}
if (!isNullOrEmpty(registry)) {

// Check if plugin matches registry format: 'registryURL/identifier'
Matcher referenceMatcher = PLUGIN_WITH_REGISTRY_PATTERN.matcher(plugin);
if (referenceMatcher.matches()) {
// Workaround; plugin broker expects an 'id' field for each plugin, so it's necessary to split
// the URL arbitrarily. See issue: https://github.com/eclipse/che/issues/13385
String registry = referenceMatcher.group("registry");
String id = referenceMatcher.group("id");
URI registryURI = null;
try {
registryURI = new URI(registry);
} catch (URISyntaxException e) {
Expand All @@ -140,9 +155,11 @@ public ExtendedPluginFQN parsePluginFQN(String plugin) throws InfrastructureExce
"Plugin registry URL '%s' is invalid. Problematic plugin entry: '%s'",
registry, plugin));
}
return new ExtendedPluginFQN(registryURI, id, null, null, null);
}

return new ExtendedPluginFQN(registryURI, id, publisher, name, version);
// Failed to match above options.
throw new InfrastructureException(format(INCORRECT_PLUGIN_FORMAT_TEMPLATE, plugin));
}

private String[] splitAttribute(String attribute) {
Expand Down
Loading