Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Apply default certificate when downloading ZIP projects #1014

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Mar 4, 2021

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

What does this PR do?

Fixes downloading of zipped projects when creating a workspace.
Now the default system certificate is used when downloading zip with curl.

What issues does this PR fix or reference?

eclipse-che/che#19120

How to test this PR?

Devfile

---
apiVersion: 1.0.0
metadata:
  name: test-import-zip
projects:
  -
    name: theia-master
    source:
      type: zip
      location: "https://github.com/eclipse-theia/theia/archive/master.zip"
components:
  - type: cheEditor
    alias: che-theia
    reference: https://raw.githubusercontent.com/chepullreq4/pr-check-files/master/che-theia/pr-1014/che_theia_meta.yaml
    memoryLimit: 1G

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=stable

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #1014 (254094d) into master (bbf4dc6) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   64.72%   64.58%   -0.14%     
==========================================
  Files          68       68              
  Lines        2466     2471       +5     
  Branches      391      392       +1     
==========================================
  Hits         1596     1596              
- Misses        860      865       +5     
  Partials       10       10              
Impacted Files Coverage Δ
plugins/workspace-plugin/src/ca-cert.ts 0.00% <0.00%> (ø)

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 bbf4dc6...254094d. Read the comment docs.

@che-bot
Copy link
Contributor

che-bot commented Mar 4, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia quay.io/crw_pr/che-theia:1014
che-theia-endpoint-runtime-binary quay.io/crw_pr/che-theia-endpoint-runtime-binary:1014

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Mar 4, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia quay.io/crw_pr/che-theia:1014
che-theia-endpoint-runtime-binary quay.io/crw_pr/che-theia-endpoint-runtime-binary:1014

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@sunix
Copy link
Contributor

sunix commented Mar 5, 2021

Would that be possible to use the env variable NODE_EXTRA_CA_CERTS (in theia editor meta.yaml) with the right certificate paths and see if it works? if it does, it may fix this issue as well:
eclipse-che/che#19168 (comment)

@vitaliy-guliy
Copy link
Contributor Author

Would that be possible to use the env variable NODE_EXTRA_CA_CERTS (in theia editor meta.yaml) with the right certificates path and see if it works ? if it does, it may fix this issue as well:
eclipse/che#19168 (comment)

It's a bit different.
The problem with downloading of ZIP packages was because curl was not using default certificates. Default certificate (it's a bundle of certificates) is necessary for https://github..., https://someotherserver, and for other server, that are normally working and for which we don't care which certificate is used. Each system has more that 200 certificates by default.

@sunix
Copy link
Contributor

sunix commented Mar 5, 2021

for that use case, it is trying to download a file from the devfile registry, so it may be related. But if we are calling curl ... yeah it won't work. But why are we calling curl and not directly downloading the files ... :/

@vitaliy-guliy
Copy link
Contributor Author

for that use case, it is trying to download a file from the devfile registry, so it may be related. But if we are calling curl ... yeah it won't work. But why are we calling curl and not directly downloading the files ... :/

@benoitf some time ago proposed to rework it a bit on using axios to download zips. I'm making a note about that improvement, next to #1014 (comment)

@benoitf
Copy link
Contributor

benoitf commented Mar 8, 2021

Hello, is the fix should be part of #963 as well ?

@sunix
Copy link
Contributor

sunix commented Mar 8, 2021

Hello, is the fix should be part of #963 as well ?

To me, #963 should not be done in Che theia, but editor agnostic.

@vitaliy-guliy
Copy link
Contributor Author

Hello, is the fix should be part of #963 as well ?

Yes, adding it to #963

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.

None yet

5 participants