CODENVY-502 allow to create factories by providing custom parameters #1247

benoitf · 2016-05-13T08:47:15Z

Change-Id: I0c9554ac6d767e110f9bc39c30e2fb2712ad413f
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

benoitf · 2016-05-13T08:49:42Z

benoitf · 2016-05-13T08:50:44Z

FYI, in the JIRA issue there is a video

skabashnyuk · 2016-05-13T09:15:47Z

-1 For the Factory Service.
I don't like idea to add to generic Factory service github specific.
May be it's part of Github Service or GithubFactoryService.

benoitf · 2016-05-13T09:17:19Z

@skabashnyuk it can't be in github service as github is only deployed on workspace agent

skabashnyuk · 2016-05-13T09:17:58Z

ok GithubFactoryService.
Also FactoryServiceClient should not contains Github sprcific

benoitf · 2016-05-13T09:18:37Z

GithubFactoryService with /factory/github mapping is ok ?

mshaposhnik · 2016-05-13T09:21:26Z

Dont forget that github is a plugin. Some custom builds theoretically may not support it.

UPD: we just allowed to create factories with any source type (and gihub too). See #1248

skabashnyuk · 2016-05-13T09:21:58Z

Not sure yet. BTW why you need that rest method? Why don't you create factory on client ?

benoitf · 2016-05-13T09:23:54Z

@skabashnyuk because logic is server side

skabashnyuk · 2016-05-13T09:29:19Z

Can you describe a bit more desired parameters and desired behaviour of your rest method? What configuration of factory will be?

benoitf · 2016-05-13T09:29:38Z

@mshaposhnik it's related to pure git, not github. You don't need github plugin in the agent.

benoitf · 2016-05-13T09:30:12Z

@skabashnyuk parameter is listed in PR. It's to create a factory based on a github url

garagatyi · 2016-05-13T09:30:22Z

-1 to include github specific features in common API

garagatyi · 2016-05-13T09:32:09Z

...er/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java

+        }
+
+        // use regexp to find repository details (repository name, project name and branch and subfolder)
+        final Pattern GITHUB_PATTERN = Pattern.compile("^(?:http)(?:s)?(?:\\:\\/\\/)github.com/(.*?)/(.*?)(?:/tree/(.*?)(?:/(.*?))?)?$");


Compilation of pattern can be improved by making it static

BTW it will be moved to another class

skabashnyuk · 2016-05-13T09:33:14Z

BTW it should be POST

benoitf · 2016-05-13T09:36:38Z

@skabashnyuk I don't see why it should be more POST

skabashnyuk · 2016-05-13T09:37:14Z

I thought it create new factory, NO?

garagatyi · 2016-05-13T09:37:53Z

I don't think that we should allow such API for factory. This task should be done by client, not API. It can be done by Java client or Js, but it definitely should not be in the API

benoitf · 2016-05-13T09:38:06Z

@skabashnyuk it is indempotent

skabashnyuk · 2016-05-13T09:52:00Z

-1 for this service.
The whole use case what you are trying to solve can help to understand the way better way of implementation.

benoitf · 2016-05-13T17:13:24Z

I have reworked to allow "pluggable" factory resolver. It's now based on plugins
to sum-up : no github introduced directly but allow to plug github, etc through extension mechanisms

codenvy-ci · 2016-05-14T01:25:25Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/578/

skabashnyuk · 2016-05-14T21:11:27Z

I don't get it. The whole picture how it will work is unclear for me.

benoitf · 2016-05-16T07:16:08Z

@skabashnyuk
the whole picture is quite easy and described in the specification document
the idea is to have factory model based without creating/storing them. Like the previous "non-encoded factories".
Factories data will be stored inside the repository, not in codenvy database.

skabashnyuk · 2016-05-16T07:44:12Z

the whole picture is quite easy and described in the specification document

Sorry. Not for me.
It's looks like your vision of technical solution for some sort of user-story.
But initial use case is not described. The missing part is the problem what we are trying to solve.
BTW it may looks like we may need to resurect "non-encoded " factory.

benoitf · 2016-05-16T07:52:37Z

I can explain everything you want.
The referenced specification document is written by Tyler
Usecase is the title of this pull request: create factories "on demand" based on a github repository for example if I have a link to a github repository.
This PR enable extensibility which is not available for now by allowing to plug external resolvers and then enhance factories through plugins. So we could extend this for other stuff.

skabashnyuk · 2016-05-16T09:18:43Z

I think solution should be more general like non-encoded factory.

TylerJewell · 2016-05-16T09:21:33Z

We do not have non encoded factory. We have this resolver. The comments should focus on the code readiness. What is left to be improved?

benoitf · 2016-05-16T09:21:48Z

it's a general solution. And it allows to plug server side logic which is required

skabashnyuk · 2016-05-16T12:53:33Z

And this functionality has no github specific?

benoitf · 2016-05-16T12:56:02Z

it has specific part for github that will be provided by a plugin.
(like extracting branch, subfolder, etc from a github url)

?url=https://github.com/ --> handle by github plugin (if enabled else it will provide error like today)
?url=http://foo.com --> error as no plugin is handling (but may work if someone write a plugin for this)

skabashnyuk · 2016-05-16T13:25:39Z

I would like to understand the scope of this two PR

codenvy/codenvy#195
#1247

Acceptance of
http://beta.codenvy.com/f/?url=sourcodeurl
Without of Github specific?

A couple of thougts.
Puting codenvy.factory.json in repository and using it may intruduce some complexity.
Like we need to have it before workspace creating and can't use cloning, for instance
some vcs providers may not introduce web interface. Other question authentication on
vcs hosting. Are we going to provide this fucntionality for public projects?

What if we provide 3 types of factory?
Current. Most advanced

http://beta.codenvy.com/f/?id=i-123123

Realy realy simple. Jut to play with because with project type blank it's almoust useles.
2. http://beta.codenvy.com/f/?source=sourcodeurl

Allow to users specify external factory json with source authoreplacemet. Almoust identical to 1.
3. http://beta.codenvy.com/f/?factory.json=linktojson

In 2-3 cases during acceptance we will crate persistant factory and continue acceptance as usual.
I think we can't miss this step. AFAIK some part of ide may reuse factory id during workspace liftime

benoitf · 2016-05-16T13:43:56Z

The goal is that we don't need persistent factory IN codenvy for 2 and 3. (if 100 users click on a url factory link it would create 100 dummy factories for nothing as persistent state is on the repository)
IDE is receiving factory data without id (this is already done today in my PR)

skabashnyuk · 2016-05-16T13:46:20Z

ok. So 2-3 are just sending to dashboard and dashboard do the business. Correct?

benoitf · 2016-05-16T13:48:59Z

no business is done server side

skabashnyuk · 2016-05-16T13:51:40Z

Not clear for me why.
AFAIK we are accepting factory in dashboard - no?

benoitf · 2016-05-16T14:01:39Z

FactoryServlet checks if factory is valid before redirecting to dashboard so factory is checked first server side.
then FactoryServlet redirect user to dashboard which create/start workspace which then delegates to IDE the projects acceptance and IDE acceptance
(dashboard and IDE don't do any logic about factory parameters, they just execute the data from the factory object returned from the server that they share)

skabashnyuk · 2016-05-16T14:09:40Z

So we just missing the part on dashboard how to get from 2-3 format to real Factory object ?

benoitf · 2016-05-16T14:10:29Z

it asks server side

skabashnyuk · 2016-05-16T14:13:13Z

from concept point of view
So we just missing the part on dashboard how to get from 2-3 format to real Factory object ?
Correct?

benoitf · 2016-05-16T14:13:47Z

there is no missing part as it's implemented in my PR

skabashnyuk · 2016-05-16T14:24:25Z

Not yet. I'm not get the whole picture. And you propose only github specific service.
Let's talk about why we need such kind of service and its obligation and parameters.

gazarenkov · 2016-05-19T08:36:37Z

As I stated in email thread I need the big picture for factories, so we need a specs after that we discuss and consider this PR.

benoitf · 2016-05-19T12:17:03Z

Pull request rewritten
please review

in addition, diagram to understand details

codenvy-ci · 2016-05-19T13:17:12Z

Build # 628 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/628/ to view the results.

garagatyi · 2016-05-20T11:07:19Z

...re-ide-api/src/main/java/org/eclipse/che/ide/api/factory/ParametersFactoryServiceClient.java

+     * @param callback
+     *         callback which return valid JSON object of factory or exception if occurred
+     */
+    void getFactory(@NotNull Map<String, String> factoryParameters, boolean validate, @NotNull AsyncRequestCallback<Factory> callback);


Have you considered usage of promises here? AFAIK it is default codestyle for IDE now

I used the same pattern than FactoryServiceClient.

For new code we prefer to use promises.

maybe you prefer but this rule is not written anywhere
so, it can't be mandatory

https://github.com/eclipse/che/blob/826353e6ab575c8aca0e215d656409fb4fd43bda/core/ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/importer/AbstractImporter.java#L57-L57

MachineServiceClient
MachineServiceClientImpl
for example

I also upgrade the other getFactory method to promise

codenvy-ci · 2016-05-21T13:10:25Z

Build # 660 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/660/ to view the results.

benoitf · 2016-05-22T13:28:23Z

so we're good now ?

garagatyi · 2016-05-22T14:32:11Z

LGTM

skabashnyuk · 2016-05-23T07:08:35Z

ок

… mechanism based on url parameters Change-Id: I0c9554ac6d767e110f9bc39c30e2fb2712ad413f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf mentioned this pull request May 13, 2016

CODENVY-502 allow to create factories by providing github URL codenvy/codenvy#195

Merged

garagatyi reviewed May 13, 2016
View reviewed changes

benoitf changed the title ~~CODENVY-502 allow to create factories by providing github URL~~ CODENVY-502 allow to create factories by providing custom parameters May 19, 2016

benoitf force-pushed the CODENVY-502 branch from 2dfb10f to b6ab129 Compare May 19, 2016 09:23

garagatyi reviewed May 20, 2016
View reviewed changes

benoitf force-pushed the CODENVY-502 branch from b6ab129 to fffa13c Compare May 21, 2016 11:54

benoitf force-pushed the CODENVY-502 branch from fffa13c to 12d3f2c Compare May 23, 2016 07:26

CODENVY-502 allow to create factories object by providing a pluggable…

f3bc97b

… mechanism based on url parameters Change-Id: I0c9554ac6d767e110f9bc39c30e2fb2712ad413f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf force-pushed the CODENVY-502 branch from 12d3f2c to f3bc97b Compare May 23, 2016 07:27

benoitf merged commit f3bc97b into master May 23, 2016

benoitf deleted the CODENVY-502 branch May 23, 2016 07:27

CODENVY-502 allow to create factories by providing custom parameters #1247

CODENVY-502 allow to create factories by providing custom parameters #1247

Conversation

benoitf commented May 13, 2016

benoitf commented May 13, 2016 • edited Loading

benoitf commented May 13, 2016

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

mshaposhnik commented May 13, 2016 • edited Loading

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

benoitf commented May 13, 2016

garagatyi commented May 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

skabashnyuk commented May 13, 2016

garagatyi commented May 13, 2016

benoitf commented May 13, 2016

skabashnyuk commented May 13, 2016

benoitf commented May 13, 2016

codenvy-ci commented May 14, 2016

skabashnyuk commented May 14, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

TylerJewell commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016

benoitf commented May 16, 2016

skabashnyuk commented May 16, 2016 • edited Loading

gazarenkov commented May 19, 2016

benoitf commented May 19, 2016

codenvy-ci commented May 19, 2016

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

azatsarynnyy May 20, 2016 • edited Loading

Choose a reason for hiding this comment

benoitf May 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented May 21, 2016

benoitf commented May 22, 2016

garagatyi commented May 22, 2016

skabashnyuk commented May 23, 2016

benoitf commented May 13, 2016 •

edited

Loading

mshaposhnik commented May 13, 2016 •

edited

Loading

skabashnyuk commented May 16, 2016 •

edited

Loading

azatsarynnyy May 20, 2016 •

edited

Loading

benoitf May 20, 2016 •

edited

Loading