theia-ide / theia-apps

Theia applications examples - docker images, desktop apps, packagings
Apache License 2.0
1.04k stars 346 forks source link

Added the libraries that are the prerequisites for theia-ide.org #398

Closed dhruvalpatel007 closed 4 years ago

dhruvalpatel007 commented 4 years ago

Problem:

Steps to Reproduce

Note: The above problem should be same for mostly all the VS Code extensions that read/write data.

Solution:

dhruvalpatel007 commented 4 years ago

@akosyakov, @vince-fugnitto .Can you please review it and update me? Thanks.

dhruvalpatel007 commented 4 years ago

@vince-fugnitto, yes ideally we should change all the images that build theia using libs. If you are fine with the changes I can go ahead and update those files.

vince-fugnitto commented 4 years ago

@vince-fugnitto, yes ideally we should change all the images that build theia using libs. If you are fine with the changes I can go ahead and update those files.

I think it'd be good yes, we can potentially resolve similar issues (as to the one you've experienced) for all the images.

dhruvalpatel007 commented 4 years ago

@vince-fugnitto, thanks for the quick review and response I will update all the files accordingly.

vince-fugnitto commented 4 years ago

@vince-fugnitto, thanks for the quick review and response I will update all the files accordingly.

Please squash your commits, and resolve the 'dco check' by including a sign-off before we can complete the review.

dhruvalpatel007 commented 4 years ago

@vince-fugnitto , considering this as my first open source commit. Could you please help me on what exactly should I do?. I tried to do everything that is mentioned in DCO details. But not sure what went wrong.

vince-fugnitto commented 4 years ago

@vince-fugnitto , considering this as my first open source commit. Could you please help me on what exactly should I do?. I tried to do everything that is mentioned in DCO details. But not sure what went wrong.

Sure, the main issue is you have multiple commits and some are also not signed off. I'd advise to squash your pull-request into a single commit, and make sure to include a sign-off. The dco check will fail if any one of the commits is not signed-off properly.

dhruvalpatel007 commented 4 years ago

@vince-fugnitto , thanks fir the help. I have only changed the Dockerfile with Alpine images. Others I am not modifying right now. I will try to test those and make changes only if needed. Other images are using differ distros and they contain build tool kit. So I dont want to add anything more if not needed.

vince-fugnitto commented 4 years ago

@vince-fugnitto , thanks fir the help. I have only changed the Dockerfile with Alpine images. Others I am not modifying right now. I will try to test those and make changes only if needed. Other images are using differ distros and they contain build tool kit. So I dont want to add anything more if not needed.

Sounds good! Please ping me when you’re ready for another round of review after cleaning up the pull-request.

marcdumais-work commented 4 years ago

Hi @dhruvalpatel007 ,

it looks like maybe you had little mishap with your PR branch. Your changes are still there, but some of this repo's commit are re-written on your PR branch, each with a different SHA vs in this repo.

Can I help you fix it?

I think the easiest way is for you to add me as a contributor on your fork of this repo, (where this PR originates). I can then quickly fix your branch and push back the result on your repo, preserving your authorship for the squashed commit containing your changes.

dhruvalpatel007 commented 4 years ago

@marcdumais-work, I have added you to the PR. Let me know if you need any assistance.

marcdumais-work commented 4 years ago

@dhruvalpatel007 - Ok, done. The CI will probably fail because of a limitation with Travis CI (the GitHub token will not be in place). But this should be ready for @vince-fugnitto to have a closer look and maybe validate locally.

Thanks for this contribution!

vince-fugnitto commented 4 years ago

@dhruvalpatel007 - Ok, done. The CI will probably fail because of a limitation with Travis CI (the GitHub token will not be in place). But this should be ready for @vince-fugnitto to have a closer look and maybe validate locally.

Thanks for this contribution!

@marcdumais-work should we rebase the pull-request against master, I noticed the travis build passed but it is still using node 10.

marcdumais-work commented 4 years ago

@marcdumais-work should we rebase the pull-request against master, I noticed the travis build passed but it is still using node 10.

I rebased at the same time I fixed the branch. I think the Travis report that says "node 10" are due to this

dhruvalpatel007 commented 4 years ago

@marcdumais-work, thanks for the help with commits. @vince-fugnitto thanks for the inputs. Should I close this PR and create new one for other changes if need or keep this open till I test it? I haven't got a chance to test others yet.

marcdumais-work commented 4 years ago

@dhruvalpatel007 I will merge this contribution now. Please open a fresh PR for further changes you may have in mind. Thanks again!