WS master should send plugin IDs instead of plugins metas to unified broker #12998

amisevsk · 2019-03-28T06:05:42Z

What does this PR do?

Makes ws master send plugin FQNs to broker instead of downloaded meta.yamls.

Draft PR until eclipse-che/che-plugin-broker#40 is merged.

What issues does this PR fix or reference?

redhat-developer/rh-che#1307; related to #12908

garagatyi

Looks good!

garagatyi · 2019-03-28T09:17:39Z

...kspace/src/test/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParserTest.java

+  }
+
+  @Test
+  public void shouldParseAllPluginsAndEditor() throws Exception {


I know that you took these cases from another test class. But could you rework this test to test with data provider? It would looks way simpler and could include all the non exception cases like what you have in shouldReturnEmptyListWhenNoPluginsOrEditors.
Apart from making code more concise and simplify its support you could split cases where different bugs would not fail the same test case as it is now with this test.
E.g.

case with registry

case with registry that has path /plugins

case with registry that has path not equal /plugins

case with registry that has multi-level path

case without registry

case with null editor

case with null plugins

case with multiple plugins

etc

To simplify matching of results you can use assertEqualsnoOrder(actual.toArray(), expected.toArray()) (please excuse not precise names of methods if any)

garagatyi · 2019-03-29T11:53:16Z

Please take into account that after merging this PR we will need to adapt RhChe https://github.com/redhat-developer/rh-che/pull/1192/files#diff-d1c7cee2e326bd168cb76031bef4ef0cR197

amisevsk · 2019-04-05T04:47:23Z

Marked as ready for review since it works with locally built plugin-broker, but should not be released until eclipse-che/che-plugin-broker#46 is merged and a new image is available.

amisevsk · 2019-04-05T04:48:07Z

For testing, images amisevsk/che-unified-plugin-broker:v0.15.2 and amisevsk/che-init-plugin-broker:v0.15.2 can be used.

sleshchenko

Overall looks good to me.

Please take a look my inline comments

sleshchenko · 2019-04-05T05:44:02Z

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

@@ -65,10 +67,10 @@
 @Beta
 public abstract class BrokerEnvironmentFactory<E extends KubernetesEnvironment> {

-  private static final String CONFIG_MAP_NAME_SUFFIX = "broker-config-map";
+  @VisibleForTesting protected static final String CONFIG_MAP_NAME_SUFFIX = "broker-config-map";


Should it be protected for testing? Maybe package private will be enough?

Good point, moved to package private.

sleshchenko · 2019-04-05T05:45:30Z

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

@@ -106,7 +111,7 @@ public BrokerEnvironmentFactory(
   * @return kubernetes environment (or its extension) with the Plugin broker objects
   */
  public E create(
-      Collection<PluginMeta> pluginsMeta, RuntimeIdentity runtimeID, BrokersResult brokersResult)
+      Collection<PluginFQN> pluginFQNs, RuntimeIdentity runtimeID, BrokersResult brokersResult)


Please update java doc according to your changes

As far as I see it is not related to this PR, but it would be cool if you fix it: brokersResult is unused.

Updated java doc and removed brokersResult -- good catch that must be leftover from the merging of brokers.

sleshchenko · 2019-04-05T05:50:34Z

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

@@ -87,13 +90,15 @@ public BrokerEnvironmentFactory(
      AgentAuthEnableEnvVarProvider authEnableEnvVarProvider,
      MachineTokenEnvVarProvider machineTokenEnvVarProvider,
      @Named("che.workspace.plugin_broker.unified.image") String unifiedBrokerImage,
-      @Named("che.workspace.plugin_broker.init.image") String initBrokerImage) {
+      @Named("che.workspace.plugin_broker.init.image") String initBrokerImage,


Just want to mention that this class is abstract and using @Inject and @Named do nothing.

I'm OK with leaving it since it may be used as a recommendation for implementations. But on another hand, it's one more place where these values for @Named are copied/pasted.

Tha's what I get for writing code to fit what's already there :)

Removed annotations.

sleshchenko · 2019-04-05T05:53:38Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java

+@Beta
+public class PluginFQNParser {
+
+  public Collection<PluginFQN> parsePlugins(Map<String, String> attributes)


Since it's a public method it would be good to have few lines as java doc here

sleshchenko · 2019-04-05T06:02:43Z

...kspace/src/test/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParserTest.java

+
+  @Test(
+      expectedExceptions = InfrastructureException.class,
+      expectedExceptionsMessageRegExp = "Invalid.*duplicated")


The full expected message is String.format("Invalid Che tooling plugins configuration: plugin %s:%s is duplicated", ...) and I think it makes sense to check full message to make sure that message is formatted correctly (message contains ID:VERSION but not other values). I started to ask it after I saw that Che Server respond with errors where messages are wrong formatted, and it's pretty easy to make mistake here especially during refactoring.
WDYT?
It makes less sense to check the full message that is not formatted but still makes sense. It's like double fields for putting a password (password, confirm password). Also, if the message becomes formatted - then the author should change tests accordingly, otherwise it would be changed at all.

Yeah it makes sense, I usually default to keeping the match as general as possible (only to ensure exception is being thrown for the right reason) but validating that plugin id and version are included makes a lot of sense.

sleshchenko · 2019-04-05T06:13:36Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/PluginFQNParser.java

+
+    List<PluginFQN> metaFQNs = new ArrayList<>();
+    if (!isNullOrEmpty(pluginsAttribute)) {
+      String[] plugins = pluginsAttribute.split(" *, *");


*, * makes sure that empty spaces will be removed between plugins but it does not remove empty spaces before the first plugin and after the last plugin. Maybe it would be safer to split by , and then trim split values.

@Test( expectedExceptions = InfrastructureException.class, expectedExceptionsMessageRegExp = "Invalid.*duplicated") public void shouldDetectDuplicatedPluginsIfTheyArePrefixedSuffixedWithEmptySpaces() throws Exception { Map<String, String> attributes = createAttributes( "", " " + formatPlugin("http://testregistry1:8080", "testplugin", "1.0"), formatPlugin("http://testregistry2:8080", "testplugin", "1.0") + " "); parser.parsePlugins(attributes); }

Good catch -- done. And thanks for the test case, I've added it to the suite.

sleshchenko · 2019-04-05T06:14:20Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/model/PluginFQN.java

+  }
+
+  @Override
+  public boolean equals(Object obj) {


Please add toString as well. It may be useful in tests

sleshchenko · 2019-04-05T06:15:45Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsplugins/model/PluginFQN.java

@@ -53,4 +57,28 @@ public String getVersion() {
  public void setVersion(String version) {
    this.version = version;
  }
+
+  @Override
+  public int hashCode() {


hashCode might look simpler like

@Override public int hashCode() { return Objects.hash(getRegistry(), getId(), getVersion()); }

But I'm OK with current way as well.

The pitfalls of auto-generated code. Fixed.

amisevsk · 2019-04-05T17:15:02Z

ci-test

che-bot · 2019-04-05T20:29:18Z

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

amisevsk · 2019-04-10T20:35:25Z

Waiting on a release of Che plugin broker to mirror the necessary changes.

Do not merge

garagatyi · 2019-04-15T09:19:48Z

@amisevsk I see that there is a release with necessary changes but it is done manually instead of by CI and container image is not pushed to Docker hub. Do you plan to push them? Do you need help with that?

…tas. Instead of downloading metas from plugin registry and passing them to plugin broker, provide plugin broker with serialized plugin fully qualified names and allow broker to download necessary metas. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2019-04-15T18:16:56Z

@garagatyi Thanks for the help, I couldn't figure out the CI backing plugin-broker releases.

Bumped broker version in che.properties, squashed all commits, and rebased on current master. Tested locally and everything works with FQN changes.

amisevsk requested a review from garagatyi March 28, 2019 06:05

garagatyi reviewed Mar 28, 2019

View reviewed changes

amisevsk mentioned this pull request Apr 5, 2019

Fix validation devfile for components with id like gist url. #13056

Merged

amisevsk marked this pull request as ready for review April 5, 2019 04:46

amisevsk requested review from metlos and sleshchenko as code owners April 5, 2019 04:46

amisevsk mentioned this pull request Apr 5, 2019

Rename id field of chePlugin/cheEditor component in Devfile #13062

Closed

sleshchenko approved these changes Apr 5, 2019

View reviewed changes

amisevsk force-pushed the plugin-broker-ids branch from bbe85d2 to 53a9065 Compare April 5, 2019 15:22

amisevsk requested a review from ibuziuk as a code owner April 5, 2019 15:22

sleshchenko changed the title ~~WS master should send plugin IDs instead of plugins metas to unified broker~~ WIP WS master should send plugin IDs instead of plugins metas to unified broker Apr 11, 2019

sleshchenko changed the title ~~WIP WS master should send plugin IDs instead of plugins metas to unified broker~~ WS master should send plugin IDs instead of plugins metas to unified broker Apr 11, 2019

garagatyi approved these changes Apr 15, 2019

View reviewed changes

amisevsk force-pushed the plugin-broker-ids branch from 53a9065 to 0516bd2 Compare April 15, 2019 18:13

amisevsk merged commit 0adb360 into eclipse-che:master Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WS master should send plugin IDs instead of plugins metas to unified broker #12998

WS master should send plugin IDs instead of plugins metas to unified broker #12998

amisevsk commented Mar 28, 2019

garagatyi left a comment

garagatyi Mar 28, 2019

garagatyi commented Mar 29, 2019

amisevsk commented Apr 5, 2019

amisevsk commented Apr 5, 2019

sleshchenko left a comment

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

sleshchenko Apr 5, 2019

amisevsk Apr 5, 2019

amisevsk commented Apr 5, 2019

che-bot commented Apr 5, 2019

amisevsk commented Apr 10, 2019

garagatyi commented Apr 15, 2019

amisevsk commented Apr 15, 2019

WS master should send plugin IDs instead of plugins metas to unified broker #12998

WS master should send plugin IDs instead of plugins metas to unified broker #12998

Conversation

amisevsk commented Mar 28, 2019

What does this PR do?

What issues does this PR fix or reference?

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Mar 29, 2019

amisevsk commented Apr 5, 2019

amisevsk commented Apr 5, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk commented Apr 5, 2019

che-bot commented Apr 5, 2019

amisevsk commented Apr 10, 2019

garagatyi commented Apr 15, 2019

amisevsk commented Apr 15, 2019