Add a shebang line
If you want to be able to run your program without explicitly specifying the interpreter, then you should make the script executable and start the script with a #!
line:
#!/bin/sh
Usage reporting
If the arguments aren't specified, show how to use the program (and exit with an error status):
if [ -z "$1" ] || [ -z "$2" || [ "${3:-}" ]
then
echo "Usage: $0 <username> <password>" >&2
exit 1
fi
I've redirected the error message to the standard error stream, so it will be seen; and I've replaced the Bash test -o
with a more portable alternation of test
commands.
It's up to you whether an empty password (as opposed to an unset password) is allowable.
Care with variable expansions
You've been pretty diligent about quotes (well done), but there is one place where you might get tripped up:
fulltoken=`curl ... "{\"password\":\"$2\"}" ...`
Could $2
contain "
? If so, you might want to escape it (${2//\"/\\\"}
) or detect it and abort before executing curl
.
The if $?
antipattern
Instead of running a command and then separately testing the status, it's more idiomatic to use a single if
:
if echo "$fulltoken" | grep authToken >/dev/null 2>&1
then
...
fi
A couple of further observations on this particular line: you could replace echo
with a <<<
redirection; you could replace the grep
with testing against a substituted string (both of these involve Bash extensions, so you might prefer to lean towards the portability above):
if <<<"$fulltoken" grep -q authToken 2>/dev/null
if [ "$fulltoken" != "${fulltoken/authToken}" ]
Related to this is
Keep the error handling close by
The program has more than one structure that looks like
if some_test
then
...
lots
...
of
...
code
...
here
...
if second_test
then
....
more
code
...
else
echo "INNER FAIL"
fi
else
echo "FAIL"
fi
This can make it harder to relate the else
clause to the correct if
. My usual approach to this is to define a die
function:
die(){
echo "$@" >&2
exit 1
}
Then the structure above becomes more linear and less indented:
some_test || die "FAIL"
...
lots
...
of
...
code
...
here
...
second_test || die "INNER FAIL"
....
more
code
...
This might not work for you if you want to report more than one problem per run, but it's certainly something to consider and possibly adapt for your purposes.
Use the simplest tool for the job
Although awk
can pick out fields from a line, that's the sole purpose of cut
, and I think it's clearer that that's all you're doing if you write like
token=$(echo "$fulltoken" | cut -d: -f8 | cut -d, -f1 | tr -d \")
Conversely, if you use awk
, use it fully - the applnId
pipeline (sed
|grep
|awk
) can be done with a single awk
invocation:
awk -F: 'BEGIN { RS = "," } $1 == "applnId" {print $2}'
Note that this is now a stricter test - it won't be fooled by a value containing applnId
, for instance; it recognises only a key of exactly applnId
.
Actually, since you have asked for JSON output, it's easier and more reliable to use jq
than pure Bash to parse it:
applnId=$(jq -r applnId <<<"$exactserverdetails")
Similarly, to test the environment name:
test "$(jq -r environment_Name <<<"$exactserverdetails")" = Dev \
|| die "Environment name is not 'Dev'"
Be more specific when reporting errors
This test doesn't tell the user much:
if [ -n $appInId ] && [ $appInId != "null" ]
then
echo "PASS"
else
echo "FAIL"
fi
Instead, I'd write:
case "${appInId-}" in
'') echo "No applnId" >&2; exit 2;;
null) echo "applnId is 'null'" >&2; exit 2;;
*) echo "applnId is valid";; # (if you must)
esac
I'd omit the last case, though - I'm not a fan of overly chatty programs.
$fulltoken
and$serverdetails
to look like, given that we're not privy to your HTTPS server. That would help validate our assumptions/observations of how those variables are subsequently used. \$\endgroup\$