Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Remove gluon.utils.download copy with verify_ssl support #193

Merged
merged 2 commits into from
Jul 26, 2018

Conversation

leezu
Copy link
Contributor

@leezu leezu commented Jul 3, 2018

Description

The problematic server now servers a valid SSL certificate. Also apache/mxnet#11546 will add the option upstream.

Checklist

Essentials

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

Changes

Comments

@leezu leezu requested a review from szha as a code owner July 3, 2018 23:06
@szha
Copy link
Member

szha commented Jul 3, 2018

We will need to wait until we update the dependency with a nightly version that includes the update in download before we merge.

@leezu
Copy link
Contributor Author

leezu commented Jul 4, 2018

Currently the updated download is not needed any more, because the third-party server was fixed. I proposed the change in mxnet as we may run into similar issues in future.

@leezu
Copy link
Contributor Author

leezu commented Jul 4, 2018

It turns out the webserver is still not fixed. The issue is that it does not serve an intermediary certificate that is needed to establish the trust from the root CA to the websites certificate. Newer browsers include the intermediary certificate used, so no error is shown when accessing the file with Firefox.

I have contacted the admin and asked to include the intermediary certificates. More info about the certificate issues can be found here https://www.ssllabs.com/ssltest/analyze.html?d=ie.technion.ac.il

@codecov
Copy link

codecov bot commented Jul 8, 2018

Codecov Report

Merging #193 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #193   +/-   ##
=======================================
  Coverage   67.84%   67.84%           
=======================================
  Files          72       72           
  Lines        7109     7109           
=======================================
  Hits         4823     4823           
  Misses       2286     2286

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 edf6231...1e0c2e4. Read the comment docs.

The problematic server now servers a valid SSL certificate.
@mli
Copy link
Member

mli commented Jul 26, 2018

Job PR-193/16 is complete.
Docs are uploaded to https://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-193/16/index.html

@szha
Copy link
Member

szha commented Jul 26, 2018

@leezu it passed. Was the issue upstream fixed?

@leezu
Copy link
Contributor Author

leezu commented Jul 26, 2018

No, I made use of the verify_ssl=False that was added to upstream mxnet.

@leezu
Copy link
Contributor Author

leezu commented Jul 26, 2018

I haven't received a response from the webmaster. They seem unwilling to fix their server.

@leezu leezu merged commit 6fa4e62 into dmlc:master Jul 26, 2018
@leezu leezu deleted the verifyssl branch July 26, 2018 18:57
paperplanet pushed a commit to paperplanet/gluon-nlp that referenced this pull request Jun 9, 2019
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.

3 participants