awscli: Add server side encryption tests #225
Conversation
run/core/awscli/test.sh
Outdated
| # tests server side encryption headers for get and put calls | ||
| function test_serverside_encryption() { | ||
| #skip server side encryption tests if HTTPS disabled. | ||
| if [ "$ENABLE_HTTPS" -ne 1 ]; then |
There was a problem hiding this comment.
Does this string comparison work? like this will evaluate to
if [ '0' -ne 1 ]; then
it is perhaps better to check like this
if [ "$ENABLE_HTTPS" != "1" ]; then
return 0
fi
There was a problem hiding this comment.
But [ "1" -eq 1 ] does numeric comparison
There was a problem hiding this comment.
It does but should we rely on the fact? because it is better to not quote it in such a case. If we quote it better to use string comparisons.
There was a problem hiding this comment.
Welcome to Bash. That is not true. You should double quote to avoid string spilling.
There was a problem hiding this comment.
Yeah but i prefer the style of if [ $ENABLE_HTTPS -ne 1 ]; then - it tells atleast to me that we are doing a numeric comparison. Using strings in first arg and then numeric is confusing while incidentally it works.
Word splitting problem only happens when you are doing things like adding variables with some static strings such as not in case of "if" doing a numerical comparison is not a problem (this can be tested locally)
$HOME/$dir/dist/bin/$file # Unquoted (bad)
"$HOME"/"$dir"/dist/bin/"$file" # Minimal quoting (good)
"$HOME/$dir/dist/bin/$file" # Canonical quoting (good)
There was a problem hiding this comment.
It actually works as per testing - perhaps because -ne is an integer operator. will fix it to avoid surprises though
There was a problem hiding this comment.
Question is how you treat the variable i.e. string type or integer type (Bash treats everything as string). The choice is yours.
There was a problem hiding this comment.
ok , updated PR to do string comparison
run/core/awscli/test.sh
Outdated
|
|
||
| # main handler for all the tests. | ||
| main() { | ||
| test_serverside_encryption_error | ||
| return |
There was a problem hiding this comment.
A return here looks out of place..
There was a problem hiding this comment.
that wasn't intended. updated PR.
Fixes #214