3
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ I don't see any database there - it looks to be querying HTTPS server(s) \$\endgroup\$ Commented Apr 28, 2017 at 7:49
  • 1
    \$\begingroup\$ It may be helpful to add comments showing approximately what you expect $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\$ Commented Apr 28, 2017 at 14:58

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot for your comments so far. Can you throw some light on the following: ` 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` \$\endgroup\$
    – me24hour
    Commented Apr 28, 2017 at 14:46
  • \$\begingroup\$ okay i will try that but will also look for your comments next week \$\endgroup\$
    – me24hour
    Commented Apr 28, 2017 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.