Closed ddelange closed 1 year ago
This creates a regression by removing a legitimate feature. It causes API requests that complete with non-2xx responses to not error. I'm going to decline this PR. You need to supply a sufficiently modern version of curl
. We might accept a PR checking the cURL version to produce a clearer error but since that's not straightforward I'd have to see the code to decide on that tradeoff.
I guess I misread the original, sounds like you do have --fail
in your cURL version. Not sure why it's not working as expected, but removing --fail
as you've done here isn't right. If set -e
changes the behavior over #!/bin/bash -e
, that's weird but fine, happy to accept that change.
Hi @bendrucker :wave:
This PR fixes two bugs:
--fail
makes non 2xx responses fail silently (curl always exists 0, see issue), and erroneously allows the script to continue despite -e
. That made the output of the script look very weird (see issue), because~ it then crashes on trying to unzip a file which was never downloaded. edit: see the next commentI removed -x
again, which just makes the script more verbose by showing which commands are run, useful for debugging (docs). Using set
is preferred over flags in shebang, ~mainly due to historical reasons I think~. edit: see the next comment
How does the diff look now?
Ah, my curl --help
posted in the issue was a bit unclear. Their man is clearer about it. Looks like it's indeed safe to have it for the file transfer. I have reverted the curl commands.
So then, the reason for the script continuing despite the errors, is that the install script is piped to plain bash
. So the shebang is ignored :) The shebang is to help your shell identify how to execute this file if you made the file executable and you simply try running it like $ install_script.sh
instead of $ bash install_script.sh
.
Final diff:
set
instead of shebangAh that makes sense. PR welcome to fix that, otherwise I'll get to it eventually.
Fixes #1865