patternbashMinor
Query database and check server details
Viewed 0 times
serverquerydatabasedetailsandcheck
Problem
I have written bash script to query database and check server details.
Please let me know if the script is as per best standards:
Please let me know if the script is as per best standards:
if [ -z "$1" -o -z "$2" ]
then
echo "FAIL : Put username and password"
else
fulltoken=`curl -s --insecure -H "Content-Type: application/json" -X POST -d "{\"username\":\"$1\",\"password\":\"$2\"}" https://apiurl`
echo $fulltoken | grep -q authToken > /dev/null 2>&1
if [ $? -eq 0 ]
then
token=`echo $fulltoken | awk -F':' '{print $8}' | awk -F',' '{print $1}' | sed 's/"//g'`
hostname=`uname -n | awk -F'.' '{print $1}'`
serverdetails=`curl -s --insecure -H "Content-Type: application/json" -H "Authorization: $token" -X GET https://apiurl`
echo $serverdetails | grep -iq "applnId"
if [ $? -eq 0 ]
then
exactserverdetails=`echo $serverdetails`
appInId=`echo $exactserverdetails | sed 's/,/\n/g' | grep "applnId" | awk -F':' '{print $2}'`
if [ -n $appInId ] && [ $appInId != "null" ]
then
echo "PASS"
else
echo "FAIL"
fi
echo $exactserverdetails | sed 's/,/\n/g' | grep "environment_Name" | awk -F':' '{print $2}' | grep -q -e "Dev"
if [ $? -eq 0 ]
then
echo "PASS"
else
echo "FAIL"
fi
else
echo "sorry server is not present"
fi
else
echo "all good"
fi
fiSolution
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
Usage reporting
If the arguments aren't specified, show how to use the program (and exit with an error status):
I've redirected the error message to the standard error stream, so it will be seen; and I've replaced the Bash
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:
Could
The
Instead of running a command and then separately testing the status, it's more idiomatic to use a single
A couple of further observations on this particular line: you could replace
Related to this is
Keep the error handling close by
The program has more than one structure that looks like
This can make it harder to relate the
Then the structure above becomes more linear and less indented:
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
Conversely, if you use
Note that this is now a stricter test - it won't be fooled by a value containing
Actually, since you have asked for JSON output, it's easier and more reliable to use
Similarly, to test the environment name:
Be more specific when reporting errors
This test doesn't tell the user much:
Instead, I'd write:
I'd omit the last case, though - I'm not a fan of overly chatty programs.
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/shUsage 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 " >&2
exit 1
fiI'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 $? antipatternInstead 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
...
fiA 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 /dev/nullif [ "$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"
fiThis 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 liketoken=$(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"
fiInstead, 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)
esacI'd omit the last case, though - I'm not a fan of overly chatty programs.
Code Snippets
if [ -z "$1" ] || [ -z "$2" || [ "${3:-}" ]
then
echo "Usage: $0 <username> <password>" >&2
exit 1
fifulltoken=`curl ... "{\"password\":\"$2\"}" ...`if echo "$fulltoken" | grep authToken >/dev/null 2>&1
then
...
fiif <<<"$fulltoken" grep -q authToken 2>/dev/nullif [ "$fulltoken" != "${fulltoken/authToken}" ]Context
StackExchange Code Review Q#162006, answer score: 2
Revisions (0)
No revisions yet.