Download release file instead of cloning repo in go template #179
Conversation
There was a problem hiding this comment.
Sorry for the delay...between this and a separate PR, looks like something funny's going on with kcov on the new Ubuntu Trusty images on Travis. Will try to figure out what's going on with that soon.
In the meanwhile, I've a few comments to share.
go-template
Outdated
| printf "Failed to clone '%s'; aborting.\n" "$GO_SCRIPT_BASH_REPO_URL" >&2 | ||
| printf "Downloading framework from '%s'...\n" "$GO_SCRIPT_BASH_DOWNLOAD_URL" | ||
| curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL" | tar -xz | ||
| if [[ ${PIPESTATUS[0]} -ne 0 ]] || [[ ${PIPESTATUS[1]:1} -ne 0 ]] ; then |
There was a problem hiding this comment.
You should just be able to say if ! curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL" | tar -xz; then ....
However, I'd like to keep the original git clone code as a fallback. So the logic would be something like:
if curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL" | tar -xz; then
# ...create scripts dir, move output directory, etc...
elif git clone ...; then
# ...clone successful output...
else
# "failed to download or clone" message
exit 1
fiAnd really, I'd want that first condition to be something like:
if command -v curl >/dev/null && command -v tar >/dev/null &&
curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL" | tar -xz; thenThere was a problem hiding this comment.
Doesn't this if ! curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL" | tar -xz; then ... depend on the pipefail option? It was my first go at this, but it continues to the rm and mv commands even though curl failed.
Is there a way in the testing framework to make the command -v statements fail?
There was a problem hiding this comment.
Hmm, you may be right about pipefail. And what I've normally done to avoid pipes throughout the framework is to use process substitutions, so maybe this would be worth a try:
if command -v curl >/dev/null && command -v tar >/dev/null &&
tar -xz <(curl -LfsS "$GO_SCRIPT_BASH_DOWNLOAD_URL); thenAs for making command -v fail, you can prefix the run command like so:
PATH="$BATS_TEST_BINDIR" run ...And check out stub_program_in_path and restore_program_in_path from lib/bats/helpers to stub out curl and tar.
go-template
Outdated
| printf "Clone of '%s' successful.\n\n" "$GO_SCRIPT_BASH_REPO_URL" | ||
| if ! mkdir -p $GO_SCRIPTS_DIR; then | ||
| printf "Faild to create scripts dir '%s'" $GO_SCRIPTS_DIR >&2 | ||
| exit 2 |
There was a problem hiding this comment.
No need for different exit codes; exit 1 is fine. Also, typo on the line above: "Faild" => "Failed"
|
I will look at the comments and submit the changes. They all look good. Would you let me know if I should change something in the TravisCI file for the checks to work? |
|
I'm going to be either busy or on a flight between now and Saturday, and may not be able to dive back in until Monday (thanks to jet lag and reuniting with friends after a month in Europe), so apologies in advance for that. Also due to my work and travel schedule, I still haven't had a chance to look into what's going on with Travis, so feel free to experiment if you like! If not, no worries; I'll take a look as soon as I can. |
|
Hey, sorry it's been a while...finally got some time to dig into the failure. When SimonKagstrom/kcov#211 gets merged, I'll kick off a new build and review. |
|
I've updated the code as you suggested. In the end, I could not use process substitution for the I still believe that it looks a bit clumsy, so feel free to come back with more changes and corrections. Thanks. |
|
Wow, I can't believe how long I've let this sit... Apologies. I've pulled your branch locally and am looking at it right now. I'm anticipating I'll end up merging your branch, then I'll create a new PR to push a commit with minor tweaks, and then release v1.6.0 so it's officially available. FWIW, I've been heads-down on other projects (devs-setup and custom-links), and have come up with some more stuff I want to push into v1.7.0 soon. One of the most fun things is more Bats helpers to test server or other processes that need to run in the background for a bit. |
Don't know why the test wasn't originally written using `TEST_GO_ROOTDIR`. As intended, it exposed some missing quotes from variables in `go-template` introduced in #179. I've removed several instances of the following comment, as I'm not sure it applies anymore. If it does, I'll make sure to restore it as needed: # Use `.*/scripts/go-script-bash` to account for the fact that `git clone` on # MSYS2 will output `C:/Users/<user>/AppData/Local/Temp/` in place of `/tmp`.
This had been hardcoded to the actual repo value in #179. The `create_fake_tarball_if_not_using_real_url` helper function and `TEST_ARCHIVE_URL` environment variable enable the test to run isolated from the network. Also tweaks `GO_SCRIPTS_DIR` to remove unnecessary braces.
Download release file instead of cloning repo in go-template (continued from #179)
It drops git clone as a mean of getting a release for
go-script-bashin favor of a download of the release file from github through curl.It uses
tarto extract the downloaded file. Tested in bsdtar and gnutar.Please review the error handling for the pipe, I'm not prolific in bash scripting and I found it to be a non robust checking.