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.

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

Fix #911 Add Eclipse Che Installer #913

Merged
merged 1 commit into from Jan 15, 2018

Conversation

dgolovin
Copy link
Contributor

@dgolovin dgolovin commented Oct 11, 2017

image

@dgolovin
Copy link
Contributor Author

test this

@codecov-io
Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #913 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
- Coverage   93.08%   93.05%   -0.03%     
==========================================
  Files          42       43       +1     
  Lines        4467     4592     +125     
  Branches      493      781     +288     
==========================================
+ Hits         4158     4273     +115     
+ Misses        309      303       -6     
- Partials        0       16      +16
Impacted Files Coverage Δ
browser/model/che.js 100% <100%> (ø)
browser/model/devstudio-autoinstall.js 97.29% <0%> (-2.71%) ⬇️
browser/pages/account/controller.js 97.93% <0%> (-2.07%) ⬇️
browser/model/helpers/downloader.js 96.13% <0%> (-1.66%) ⬇️
browser/model/devstudio-p2-zip.js 64.4% <0%> (-1.12%) ⬇️
browser/model/devstudio-central-p2-zip.js 60.31% <0%> (-0.98%) ⬇️
browser/model/jbossfusekaraf.js 95.09% <0%> (-0.83%) ⬇️
browser/pages/install/controller.js 87.79% <0%> (-0.59%) ⬇️
browser/services/data.js 82.24% <0%> (-0.52%) ⬇️
browser/services/platform.js 98.85% <0%> (-0.34%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9932b0...1202048. Read the comment docs.

progress.setStatus('Installing');
let installer = new Installer(this.keyName, progress, success, failure);
return Promise.resolve().then(()=>{
return installer.exec('minishift config set memory 5GB && minishift addon enable che', this.createEnvironment());
Copy link

Choose a reason for hiding this comment

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

@LalatenduMohanty looks like we are enabling during installation. I think we need to just set memory when user click to install Che. and point to documentation to apply che.

Copy link

Choose a reason for hiding this comment

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

/cc @dgolovin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@budhrg @LalatenduMohanty Why would we call it Eclipse Che Installer then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dgolovin is right. Looks good to me. @budhrg the idea of installer is to make the process super easy for user. So this is the right thing to do as Che would be a separate entity in installer.

@dgolovin dgolovin changed the title Fix #911 Add Eclipse Che Installer Fix #911 [WIP}Add Eclipse Che Installer Oct 27, 2017
@dgolovin dgolovin changed the title Fix #911 [WIP}Add Eclipse Che Installer Fix #911 [WIP]Add Eclipse Che Installer Oct 27, 2017
@dgolovin dgolovin changed the title Fix #911 [WIP]Add Eclipse Che Installer [WIP] Fix #911 Add Eclipse Che Installer Dec 8, 2017
@dgolovin dgolovin changed the title [WIP] Fix #911 Add Eclipse Che Installer Fix #911 Add Eclipse Che Installer Jan 11, 2018
progress.setStatus('Installing');
let installer = new Installer(this.keyName, progress, success, failure);
return Promise.resolve().then(()=>{
return installer.exec('minishift config set memory 5GB && minishift addon enable che', this.createEnvironment());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dgolovin is right. Looks good to me. @budhrg the idea of installer is to make the process super easy for user. So this is the right thing to do as Che would be a separate entity in installer.

@coolbrg
Copy link

coolbrg commented Jan 12, 2018

Need rebase.

@dgolovin
Copy link
Contributor Author

Depends on #1213

@dgolovin dgolovin merged commit f1fb78b into redhat-developer-tooling:master Jan 15, 2018
@dgolovin dgolovin deleted the gh911 branch January 30, 2018 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants