HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Query database and check server details

Submitted by: @import:stackexchange-codereview··
0
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:

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
fi

Solution

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  " >&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 /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.

Code Snippets

if [ -z "$1" ] || [ -z "$2" || [ "${3:-}" ]
then
  echo "Usage: $0 <username> <password>" >&2
  exit 1
fi
fulltoken=`curl ... "{\"password\":\"$2\"}" ...`
if echo "$fulltoken" | grep authToken >/dev/null 2>&1
then
  ...
fi
if <<<"$fulltoken" grep -q authToken 2>/dev/null
if [ "$fulltoken" != "${fulltoken/authToken}" ]

Context

StackExchange Code Review Q#162006, answer score: 2

Revisions (0)

No revisions yet.