No error checking when running curl

[danp]

curl is shelled out to in various places, such as here, but it's not run with --fail and the run method doesn't do any error checking.

When a download done with curl silently fails it can lead to surprising errors such as can't convert nil into String (TypeError) from File.expand_path in add_bundler_to_load_path.

[hone]

@dpiddy what should we do here? just use the --fail flag on curl, or just provide better error messaging?

[danp]

I'd start with having run always check exit status and throw/display a general error (and internally log something) if what's being run fails. Then add --fail to all curl commands. Prefixing all commands run with run that use pipes with set -o pipefail; ... would be good too. Looks like pipe could also use an exit status check.

To get fancier with curl, I'd suggest something like:

$ curl --fail --retry 3 --retry-delay 1 --connect-timeout 3 --max-time 20 ...

This will get some retry on simple transient S3 issues.

[neersighted]

👍

[danp]

Getting bit by this again today:

$ curl https://s3.amazonaws.com/heroku-buildpack-ruby/ruby_versions.yml
<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>InternalError</Code><Message>We encountered an internal error. Please try again.</Message><RequestId>F95682256711C6D9</RequestId><HostId>lipO+wYGTyZqVPUBv//x5inR6uM8ZQWnOQopnXAX0n92ngS1t76zxTSp24t9r2uE</HostId></Error>

Causing the weirdness I linked above as well as the indication that ruby-1.9.3 is not a valid RUBY_VERSION.

[schneems]

@dpiddy, would you be willing to work this into a PR. I don't see any reason to hide this information from users, and the retry code would be icing on the cake. Seems like a good idea.

[danp]

Have a start of something here.

[hone]

@dpiddy I was trying this out but ran into this issue: set: 1: Illegal option -o pipefail on heroku

[danp]

Did you pick up the change to bash here? pipefail is a bash option.

[hone]

Nope. Let me try that.

[schneems]

It's been a few months, is your branch curl-usage-improvements in a PR-able state? Would like to either get this merged in or closed. I would think we also want to do most curls with --retry set to a non zero number.

[danp]

Probably not. Maybe at this point I'd look into putting error checking and options (such as --retry) into Fetcher and ensuring it's used everywhere.

[schneems]

@dpiddy can you look at #165 ?