Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

Initial contribution #1

Merged
merged 12 commits into from
Jun 26, 2018
Merged

Initial contribution #1

merged 12 commits into from
Jun 26, 2018

Conversation

sunix
Copy link
Contributor

@sunix sunix commented Jun 4, 2018

Provides basic factory client side clone feature

  • checking factory-id from url
  • request che factory api to get the factory definition
  • clone the projects defined in the factory definition
  • checkout branch if needed

eclipse-che/che#9837
eclipse-che/che#9221

@slemeur
Copy link

slemeur commented Jun 4, 2018

Does it handle parse checkout yet? :)

@slemeur
Copy link

slemeur commented Jun 4, 2018

Can we add a small recording with this ?

@sunix
Copy link
Contributor Author

sunix commented Jun 4, 2018

@slemeur

  1. no parse checkout at the moment, let me know if this is priority and I will create an issue
  2. yes will work on a recording today

@sunix sunix force-pushed the initialContrib branch from 7309542 to 758e3a0 Compare June 5, 2018 07:39
"theia": {
"target": "browser"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new line is here


bind(FactoryTheiaClient).toSelf().inSingletonScope();
bind(FrontendApplicationContribution).toDynamicValue(c => c.container.get(FactoryTheiaClient));
});
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

});
})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing


export interface IFactory {
workspace: IWorkspaceConfig;
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

"include": [
"src"
]
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

"theia": {
"target": "electron"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

"stream": true
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

New line is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like new line is there

});
});
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you, please, rewrite this method with async/await, and maybe split in several, it is hard to read code with so many callbacks

"name": "browser-app",
"version": "0.0.0",
"dependencies": {
"@theia/core": "latest",

Choose a reason for hiding this comment

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

Could you use 0.3.10 version everywhere, to prevent potential breaking browser-app and electron-app modules, after newer Theia realeses?

"@eclipse-che/workspace-client": "latest"
},
"devDependencies": {
"rimraf": "latest",

Choose a reason for hiding this comment

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

Use, please, version 2.6.2 or another one approved by Eclipse CQ.

},
"devDependencies": {
"rimraf": "latest",
"typescript": "latest"

Choose a reason for hiding this comment

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

Use, please, version 2.7.2 or another one approved by Eclipse CQ.

Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
@sunix
Copy link
Contributor Author

sunix commented Jun 21, 2018

@AndrienkoAleksandr @evidolob I have updated the PR with your feedback. Let me know if it works for you

README.md Outdated
3. Create a new factory from the created workspace.
4. Edit the factory and select `from a stack` and `Java Theia (OpenShift)`
5. Edit the configuration and add to the theia machine the environment variable THEIA_PLUGINS
Copy link
Member

Choose a reason for hiding this comment

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

could you please use
THEIA_EXTENSIONS (as THEIA_PLUGINS will now only work with theia plugins)

README.md Outdated
"env": {
"CHE_MACHINE_NAME": "ws/theia",
"THEIA_PLUGINS": "che-theia-factory:https://github.com/eclipse/che-theia-factory-extension.git"
Copy link
Member

Choose a reason for hiding this comment

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

could you please use
THEIA_EXTENSIONS (as THEIA_PLUGINS will now only work with theia plugins)

README.md Outdated
## Developing with the browser example

Start watching of the hello world extension.
Copy link
Member

Choose a reason for hiding this comment

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

FYI it's not hello world extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

;)

"@theia/terminal": "0.3.11",
"@theia/typescript": "0.3.11",
"@theia/workspace": "0.3.11",
"@eclipse/che-theia-factory": "^0.3.11"
Copy link
Member

Choose a reason for hiding this comment

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

carret should be removed as well
also I'm not sure this extension should use 0.3.11 version , it's the first version so more like 0.0.1 (or something like that)

"keywords": [
"theia-extension"
],
"version": "0.3.11",
Copy link
Member

Choose a reason for hiding this comment

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

0.0.1 ?


import { AxiosInstance, AxiosPromise } from 'axios';

export interface IResources {
Copy link
Member

Choose a reason for hiding this comment

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

some comments ?

import { IWorkspaceConfig } from "@eclipse-che/workspace-client";


export interface IFactory {
Copy link
Member

Choose a reason for hiding this comment

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

do we need this .d.ts ?

@@ -0,0 +1,32 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

latest here should not be used

{
"private": true,
"name": "electron-app",
"version": "1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

version should be the same on the whole project

"@theia/terminal": "latest",
"@theia/typescript": "latest",
"@theia/workspace": "latest",
"sunix-che-theia-factory-frontend-extension": "^1.0.2"
Copy link
Member

Choose a reason for hiding this comment

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

something wrong there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok let's remove the electron app what do you think ?

sunix added 6 commits June 21, 2018 13:01
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
README.md Outdated
Open http://localhost:3000 in the browser.

## Developing with the Electron example

Choose a reason for hiding this comment

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

I guess, you should remove this section, because you removed electron package.

lerna.json Outdated
@@ -0,0 +1,11 @@
{
"lerna": "2.4.0",
"version": "1.0.2",

Choose a reason for hiding this comment

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

You definded version of your extension like 0.0.1: https://github.com/eclipse/che-theia-factory-extension/pull/1/files#diff-2c9e1ab7353b6750c47de1b55b578910R6, so I think here should be the same version

@sunix
Copy link
Contributor Author

sunix commented Jun 25, 2018

@benoitf @eivantsov @AndrienkoAleksandr could you review and approve if it is ok ?

Signed-off-by: Sun Seng David TAN <sutan@redhat.com>
Signed-off-by: Sun Seng David TAN <sutan@redhat.com>

/**
* Provides basic factory client side features at startup of the browser IDE:
* - checking/retriving factory-id from URL
Copy link
Member

Choose a reason for hiding this comment

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

FYI small typo on retrieving

@sunix sunix merged commit fea05c5 into master Jun 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants