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

fetchKSSL: Fix for "status was 'SSL connect error'" #267

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Conversation

brownag
Copy link
Member

@brownag brownag commented Oct 4, 2022

Thanks to Ben Marshall @Kramdog for bringing this up / reminding me that the SSL verification was still an issue in some cases. Since jsonlite changed behind the scenes, we need to attend to fetchKSSL-related downloads.

This updates fetchKSSL() to use {curl} internally for downloading the gzipped JSON from SoilWeb. This was what was done implicitly w/ fromJSON() w/ jsonlite <1.8.1, but with the ssl_verifyhost option set in the header.

While the base R approach / default headers included in jsonlite >1.8.1 work fine on non-gov machines we occasionally encounter errors related to SSL verification when in the office or connected to VPN, i.e.
image

Since we recently set up {curl}-based approach for replacing download.file() (due to timeout and SSL limitations), we just need to use the standard soilDB curl handle (including ssl_verifyhost=0) to sort this out. The gzipped json is downloaded to a temporary file with curl, then read as a gzfile() connection into jsonlite::fromJSON(), and then the temp file is unlinked

 - use curl internally and use standard soilDB curl handle including  ssl_verifyhost=0
@brownag brownag added the SoilWeb Related to the SoilWeb / UCDavis LAWR servers, Henry Mount DB, backend functionality and public API label Oct 4, 2022
@brownag
Copy link
Member Author

brownag commented Oct 4, 2022

The broken test (https://github.com/ncss-tech/soilDB/actions/runs/3184898261/jobs/5193839591) in R devel is unrelated to the changes. See #265

@dylanbeaudette
Copy link
Member

Looks good to me, thanks!

@brownag brownag merged commit 5dc332e into master Oct 4, 2022
@brownag brownag deleted the fetchkssl-ssl branch October 16, 2022 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SoilWeb Related to the SoilWeb / UCDavis LAWR servers, Henry Mount DB, backend functionality and public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants