-
Notifications
You must be signed in to change notification settings - Fork 471
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
scripts: enhance upload_config.sh #5260
Conversation
* Adds `trap` * Prevents `tar` from directing stderr to null * Clarifies and corrects tmp directory creation
@@ -1,5 +1,6 @@ | |||
#!/usr/bin/env bash | |||
set -e | |||
trap 'echo "ERROR: ${BASH_SOURCE}:${LINENO} ${BASH_COMMAND}"' ERR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will display the line in this script from which any error originates, helpful even when a line makes an error but does not emit stderr for some reason.
@@ -33,11 +34,11 @@ SRCPATH=${SCRIPTPATH}/.. | |||
export CHANNEL=$2 | |||
export FULLVERSION=$($SRCPATH/scripts/compute_build_number.sh -f) | |||
|
|||
TEMPDIR=$(mktemp -d 2>/dev/null || mktemp -d -t "tmp") | |||
TEMPDIR=$(mktemp -d -t "upload_config.tmp.XXXXXX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was finding that when storage on my instance was full, this first call was silently failing, and then the second call would also fail, but not because of storage. All mktemp formats need a section of "XXX" to decide where to put the random ID.
The script was failing with an error of not enough Xs in template "tmp"
when the true error was out of disk. Very confusing.
Solution here is to not even try to use a default tmp format, but instead supply our own. I added the string "upload_config" to the front to make them easily identifiable and removable
TARFILE=${TEMPDIR}/config_${CHANNEL}_${FULLVERSION}.tar.gz | ||
|
||
cd $1 | ||
tar -zcf ${TARFILE} * >/dev/null 2>&1 | ||
tar -zcf ${TARFILE} * >/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not outputting standard error meant this script will just silently exit when tar
fails, meaning that it is unclear if the script passed or failed. Trapping the error will help, but it would still hide the reason, so we should display any tar
errors, since they are critical to this script.
Codecov Report
@@ Coverage Diff @@
## master #5260 +/- ##
==========================================
+ Coverage 53.78% 53.83% +0.05%
==========================================
Files 450 450
Lines 56201 56201
==========================================
+ Hits 30226 30258 +32
+ Misses 23626 23605 -21
+ Partials 2349 2338 -11 see 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments along with the changes. The changes makes sense LGTM
trap
tar
from directing stderr to nullMade these changes while I worked on getting a config uploaded which silently failed due to tar errors.