Create Unified plugin broker that merges functionality of all the plugin type specific brokers #37

garagatyi · 2019-03-20T12:06:58Z

What does this PR do?

Note: This PR is designed to be reviewed in a commit-by-commit manner. Commit message of each commit explains what was changed in the corresponding commit.

Add new unified plugin broker that should co-exist with other plugin brokers for the time being.
It accepts plugin meta.yaml entries of all the types instead of type specific as other brokers do.
It sorts meta.yaml entries depending on the type and pass them to the appropriate broker implementations.
Tested locally with config.json containing all the meta.yaml entries from the repo for manual testing of other brokers.

What issues does this PR fix or reference?

Closes redhat-developer/rh-che#1304

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

sleshchenko

LGTM

brokers/unified/broker.go

amisevsk

LGTM

davidfestal

Great. I'll be able to use the unified broker in the initContainer I create in the CRD controller. 1 container instead of 3 at workspace start. I'll rebase my current work on brokers on this change.

garagatyi · 2019-03-20T13:33:43Z

@davidfestal not sure whether it is good time to do that. We also plan to move meta download from plugin registry in unified broker. With this improvement you would be able to remove some code from CRD POC codebase

garagatyi · 2019-03-20T13:45:09Z

@davidfestal @sleshchenko @amisevsk I made plugin type matching case insensitive, please take a look and approve again

davidfestal · 2019-03-20T14:06:47Z

@davidfestal not sure whether it is good time to do that. We also plan to move meta download from plugin registry in unified broker. With this improvement you would be able to remove some code from CRD POC codebase

In fact I was already planning to open a very lightweight PR on plugin brokers with my local changes that allowed me to reuse them as is in the CRD Controller. I'll do it as soon as your current PR is merged: It included the promotion of the processPlugin() methods to public visibility, which is now provided by your PR.

And then I plan to integrate in the CRD controller new features / refactorings of the che-plugin-broker as soon as they are merged. The idea is to keep up-to-date and use standard components as much as possible.

amisevsk

LGTM still.

If we're matching case-insensitive, does it make sense to include that in some tests? E.g. the type on the added test config could be lower case.

sleshchenko

LGTM

garagatyi · 2019-03-21T09:34:35Z

If we're matching case-insensitive, does it make sense to include that in some tests? E.g. the type on the added test config could be lower case.

@amisevsk you are right, I'll need to add tests for that

garagatyi · 2019-03-22T13:14:48Z

@amisevsk I added unit tests and couple of fixes that I was needed for testing or that I discovered along the way

brokers/che-plugin-broker/broker.go

garagatyi · 2019-03-22T13:32:09Z

Guys I did some changes. I checked them on Che - everything works fine. I started workspace with plugins of each kind.
Please, add your reviews again

This broker is capable of processing Che plugins of all types. Depending on the plugin type it calls an appropriate plugin broker implementation. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Allow brokers methods reusage outside of package

a561bf9

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi requested review from davidfestal, skabashnyuk, sleshchenko and amisevsk March 20, 2019 12:06

garagatyi changed the title ~~Unified broker~~ Create Unified plugin broker that merges functionality of all the plugin type specific brokers Mar 20, 2019

sleshchenko approved these changes Mar 20, 2019

View reviewed changes

brokers/unified/broker.go Outdated Show resolved Hide resolved

amisevsk approved these changes Mar 20, 2019

View reviewed changes

davidfestal reviewed Mar 20, 2019

View reviewed changes

davidfestal approved these changes Mar 20, 2019

View reviewed changes

garagatyi mentioned this pull request Mar 20, 2019

Use single unified plugin broker for all the plugins eclipse-che/che#12942

Merged

garagatyi requested review from davidfestal, amisevsk and sleshchenko March 20, 2019 13:44

davidfestal approved these changes Mar 20, 2019

View reviewed changes

amisevsk approved these changes Mar 20, 2019

View reviewed changes

sleshchenko approved these changes Mar 21, 2019

View reviewed changes

ibuziuk mentioned this pull request Mar 21, 2019

Che Plugin Brokers - Need to update information related to creation of single Unified Plugin Broker ibuziuk/docs#2

Open

garagatyi mentioned this pull request Mar 22, 2019

Allow using the che-plugin-broker as a library in the Workspace CRD controller #38

Merged

ibuziuk approved these changes Mar 22, 2019

View reviewed changes

davidfestal reviewed Mar 22, 2019

View reviewed changes

brokers/che-plugin-broker/broker.go Show resolved Hide resolved

garagatyi requested review from davidfestal, sleshchenko, ibuziuk and amisevsk March 22, 2019 13:30

davidfestal approved these changes Mar 22, 2019

View reviewed changes

ibuziuk approved these changes Mar 22, 2019

View reviewed changes

amisevsk approved these changes Mar 25, 2019

View reviewed changes

sleshchenko approved these changes Mar 25, 2019

View reviewed changes

Oleksandr Garagatyi added 7 commits March 25, 2019 10:13

Add unified plugin broker

96042f6

This broker is capable of processing Che plugins of all types. Depending on the plugin type it calls an appropriate plugin broker implementation. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Make plugin type matching case insensitive

506570b

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Add more cases for manual testing config

85eb85f

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Make brokers interfaces to mock them

1451e5d

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Add mocks for brokers impls

72ec796

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Refactor unified broker to make it testable

2a0d22c

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Add unified broker tests

b0141e1

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the unifiedBroker branch from e3b76bb to b0141e1 Compare March 25, 2019 08:14

garagatyi merged commit 207ca2c into master Mar 25, 2019

garagatyi deleted the unifiedBroker branch March 25, 2019 08:16

garagatyi mentioned this pull request Mar 27, 2019

Cleanup brokers #42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Unified plugin broker that merges functionality of all the plugin type specific brokers #37

Create Unified plugin broker that merges functionality of all the plugin type specific brokers #37

garagatyi commented Mar 20, 2019

sleshchenko left a comment

amisevsk left a comment

davidfestal left a comment •

edited

Loading

garagatyi commented Mar 20, 2019

garagatyi commented Mar 20, 2019

davidfestal commented Mar 20, 2019

amisevsk left a comment

sleshchenko left a comment

garagatyi commented Mar 21, 2019

garagatyi commented Mar 22, 2019

garagatyi commented Mar 22, 2019

Create Unified plugin broker that merges functionality of all the plugin type specific brokers #37

Create Unified plugin broker that merges functionality of all the plugin type specific brokers #37

Conversation

garagatyi commented Mar 20, 2019

What does this PR do?

What issues does this PR fix or reference?

sleshchenko left a comment

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

davidfestal left a comment • edited Loading

Choose a reason for hiding this comment

garagatyi commented Mar 20, 2019

garagatyi commented Mar 20, 2019

davidfestal commented Mar 20, 2019

amisevsk left a comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

garagatyi commented Mar 21, 2019

garagatyi commented Mar 22, 2019

garagatyi commented Mar 22, 2019

davidfestal left a comment •

edited

Loading