3
\$\begingroup\$

I am asking for an experienced person to review my basic shell script. I do not write them at work, but I am a software developer. I am using this for my personal project that I am working on. Later, it will be integrated with Docker and a CI server maybe Jenkins/Travis CI.

I am just looking for someone to tell me if I am not following best practices or conventions, or if certain parts need to be changed/revised.

#!/bin/bash
PATH=$PATH:$(npm bin)
set -x
webapp="/Users/earzate/Development/Career/profile-page-app"
serverapp="/Users/earzate/Development/Career/profile-server-ws/app-server"

#build profile-page-app
cd $webapp
echo "Building ng page app"
ng build --prod
echo "ng page app build complete"

#build app-server
echo "Cleaning static sources"

#clean old static web files
cd "$serverapp/src/main/resources"
echo "Deletic static web resources"
rm -r static

#make new static directory
echo "Creating static directory"
mkdir static

#copy web resources from webapp into serverapp static directory
echo "Copying static resources into directory"
cp -r "$webapp/dist/."  "$serverapp/src/main/resources/static"

#copy secured objects from secured directory
#secured objects not versioned in git
cp "$serverapp/secured/name/private-key.p12" "$serverapp/src/main/resources"

echo "Building jar"
cd "$serverapp"
./gradlew build 

echo "Deploying app"
java -jar build/libs/app-server-0.0.1-SNAPSHOT.jar
\$\endgroup\$
0

3 Answers 3

Reset to default
3
\$\begingroup\$

  • Immediate security problem

    Do you know which ng, cp, rm, gradlew does the script execute? You shouldn't rely on PATH.

  • Indirect security problem

    rm and cp can fail midway, and you may end up deploying something unintended. Along the same lines, can ng produce something half-baked? Consider testing exit codes and act accordingly.

  • Every time you want to comment the behavior, consider factoring it into a well-named function, e.g.

    recreate_static_directory() {
    cd "$1"
    echo "Deletic static web resources"
    rm -r static

    echo "Creating static directory"
    mkdir static

    echo "Copying static resources into directory"
    cp -r "$webapp/dist/."  "$1/static"
    cp "$serverapp/secured/emmanuel-arzate/private-key.p12" "$1"

  • I am not sure I follow the mix of cds and fully-qualified paths.

\$\endgroup\$
2
  • \$\begingroup\$ Could you elaborate or provide some resources in order to understand how or why executing, ng, cp, rm, gradlew is a security issue? I will have ng installed globaly in my vm. I didn't know there are multiple instances of cp or rm. ./gradlew is the the gradle wrapper supplied inside the root path of my web server module, I will not have gradle installed on the vm. \$\endgroup\$ Commented Jul 22, 2017 at 0:35
  • \$\begingroup\$ Great tip on getting rid of comments. I will be updating my script accordingly . Thank you. \$\endgroup\$ Commented Jul 22, 2017 at 0:36
3
\$\begingroup\$

Consider set -e

If you ask the shell to exit when any command fails, you guard against a whole class of problems - the most obvious being a failure of cd leaving you somewhere you didn't expect to be.

Use a subshell for other-directory operations

Instead of changing working directory all over the filesystem, a cleaner and less confusing pattern I use is

(cd "$dir" && some_other_command )

Don't touch everything in static

The static directory is completely removed and rebuilt every time the script is run. That may mean that the contents always appear newer, whether they have actually changed or not. It might be sufficient to cp --preserve=timestamps to remedy that, or you might consider something more intricate.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I added the -e to exit on a failed operation. For the static folder I am removing everything because I am generating new bundled static files with hashes in order to enforce cache busting on the web browser. Everything in static folder needs to be removed. \$\endgroup\$ Commented Jul 22, 2017 at 0:09
3
\$\begingroup\$

Kudos

  • set -x Your use of this to trace your script is excellent. You might also like set -e which exits if any command fails.

Suggestions

  • no error checking What if your destination directory doesn't exist? You won't be able to cd into it or cp to it. Check that all of your assumptions are valid before proceeding.
  • spaces after hashes Your comments should start with # then a space. Leaving out the space makes it look cluttered and hard to read. Your comments seem useful in this case, but they're also somewhat redundant with the output being echod so you may want to just rely on the echos so that they don't get out of sync with the comments.
  • add a command line argument It seems like the main thing you would want to change in your script is the version number of the jar file being installed. Make that a command line argument and check that the resulting file exists early in the script.
  • organize with functions As vnp stated splitting your code into logic chunks with functions would be good. But I would start with with a function to replace your cd commands. Define a mycd or something like that which checks for the existence of the destination directory and optionally prints out where you ended up after the cd.
  • use automated checks Run your code through https://www.shellcheck.net/ and deal with any of its warnings.
\$\endgroup\$
3
  • \$\begingroup\$ Thank you chicks. I was able to check add a command line argument to set my build version for my jar. I am working to organize with functions and adding error checking in case a directory or file is missing. That shellcheck.net site is great. \$\endgroup\$ Commented Jul 22, 2017 at 0:29
  • \$\begingroup\$ I will update my question soon with updated script. \$\endgroup\$ Commented Jul 22, 2017 at 0:30
  • \$\begingroup\$ Don't update your code in the question. Code Review prefers a new question for fresh code. \$\endgroup\$ Commented Jul 22, 2017 at 1:25

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.