Improve reliability of downloading PE tarball #215
Conversation
Co-authored-by: Reid Vandewiele <reid@puppet.com>
Co-authored-by: Reid Vandewiele <reid@puppet.com>
Co-authored-by: Reid Vandewiele <reid@puppet.com>
Co-authored-by: Reid Vandewiele <reid@puppet.com>
|
I just realized something. This task is
Maybe in the future we should consider creating a |
mcka1n
changed the title
mcka1n
changed the title
|
Note: After this PR gets approved, I would like to |
tasks/download.sh
Outdated
| return 0 | ||
| else | ||
| echo "Error: Curl has failed to download the file" | ||
| echo "|_ Removing temporary file: ${tmp_file}" |
There was a problem hiding this comment.
Is this a typo?
| echo "|_ Removing temporary file: ${tmp_file}" | |
| echo "Removing temporary file: ${tmp_file}" |
tasks/download.sh
Outdated
| curl -s -f -L -o "$2" "$1" | ||
| tmp_file=$(mktemp "peadm-download.XXX") | ||
| echo "Temporary file created at: ${tmp_file}" | ||
| download_file=$(curl -s -f -L -o ${tmp_file} "$1") |
There was a problem hiding this comment.
The output of the curl command doesn't tell us if it succeeded or not. What we want to judge on is the exit code. There are three ways to do that, that I know of.
Option 1: && chaining
curl -s -f -L -o ${tmp_file} "$1" \
&& mv "${tmp_file}" "$2" \
&& return 0
# Failure to return means something failed
echo "Error: Curl has failed to download the file"
echo "Removing temporary file: ${tmp_file}"
rm "${tmp_file}"
return 1
Option 2: use an if statement
if curl -s -f -L -o ${tmp_file} "$1"; then
mv "${tmp_file}" "$2"
return 0
else
echo "Error: Curl has failed to download the file"
echo "Removing temporary file: ${tmp_file}"
rm "${tmp_file}"
return 1
fi
Option 3: check/capture $?
curl -s -f -L -o ${tmp_file} "$1"
curl_exit_code="$?"
if [[ "$curl_exit_code" = 0 ]]; then
mv "${tmp_file}" "$2"
return 0
else
echo "Error: Curl has failed to download the file; exit code ${curl_exit_code}"
echo "Removing temporary file: ${tmp_file}"
rm "${tmp_file}"
return 1
fi
I think I would probably go with option 1 because of my familiarity with it in lots of scripts from my past, but whichever of these is most readable would work great. The key bit is that we care about curl's exit code, not about its output.
There was a problem hiding this comment.
Thanks for the suggestions @reidmv I went with option No.2, I find it more readable :)
I'm waiting for the Install Github Action to finish
Co-authored-by: Reid Vandewiele <reid@puppet.com>
|
A transient agent install failure was the only problem with the smoke tests: All other agent installs in that arch, and all agent installs in other architectures and versions, worked. So, transient. LGTM! 👍 |
reidmv
changed the title
This fix makes it so that when files are downloaded, they are first downloaded to a temp file and moved into place only if the download completes and is successful. This avoids a scenario where a file can be partially downloaded, after which PEAdm thinks that the file is present and will attempt to use the partial file as-is, rather than re-downloading.
There are still ways that an incorrect file can end up being placed—namely, the download server returning HTTP 200 but the wrong file. This fix though should eliminate the problem caused by partial interrupted downloads.
Changes
curlsuccessfully downloaded the file so we can throw an error