JP Vossen on 27 Apr 2012 22:00:11 -0700

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

Re: [PLUG] First shell script for deployment...

Date: Thu, 26 Apr 2012 17:23:33 -0400

In the message dated: Thu, 26 Apr 2012 16:34:04 EDT,
The pithy ruminations from Paul Walker on
<[PLUG] First shell script for deployment...>  were:

[Contents edited for clarity and conciseness, not that I'm known for the latter...]

	You did a terrific thing by asking for constructive
	criticism. Regardless of who we are, we can all improve by this
	kind of review, and teach others by showing our work. Asking
	someone else to look at your work is often much harder than
	doing the work itself.


=>  #!/bin/bash

The following trailing '-' is voodoo against an ancient security bug:
#!/bin/bash -

That also assumes the location of bash, which may vary on non-Linuxy systems. The more portable way is:
#!/usr/bin/env bash

Having said that, I almost always use "#!/bin/bash"...

=>  # Run this from the directory **above** the public_html directory for the
=>  site you are deploying

Missing info. re version, date, author.

+1. Seems like a PITA at first, but after it saves you countless time & frustration over the long haul, you will both appreciate it and loath those scripts without it.

Ditto comments and general clarity.

=>  #create dir and clone upstream repo

You should check if the directory exists before trying to do the mkdir...that
would be good indication that you're in the wrong parent dir, or that a
previous run of the script failed.

Does there need to be a check of what user is running the script (root?
apache? nobody? JoeRandomHacker?) before proceeding?

Good points.  Here's a neat line to deconstruct.

[ -d /tmp/foo ] || mkdir -m 0700 -p /tmp/foo

	$ help test
	$ man mkdir

('help' is for internal commands, 'man' is for external commands.)

=>  mkdir public_html_deploy1

There's an assumption about the directory where the script is being run. It's
much safer to cd to an /absolute/path/to/the/working/dir at the beginning.

+, though keeping those things in *meaningfully named* variables can sometimes be useful, but also sometimes be confusing. See the *meaningfully named* part. I've renamed variables 2-3 times during development because as the code emerged different names made things more clear.

I'll even go so far as to insert "filler" words in function call to make the code read well. Like a function to prompt a user and validate using a regex:

_Prompt_User "Enter an IP Address:" \
  matching "[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}"

The "matching" part I just ignore in the function, but it make it really clear what's going on.

The script is missing checks for the return value of each step. That's a
common--and serious--failure. I usually deal with that with a shell function,
such as:

         if [ $1 != 0 ] ; then
                 # There was an error, bail out!
                 /bin/echo "$2" 1>&2
                 exit $1

There are easier ways to do this, *but* they are not as clear or user friendly.

The really easy way is 'set -e' at the point in your script after which if something fails you want to die. Or, in other words:

$ help set
-e  Exit immediately if a command exits with a non-zero status.

The other ways are variations on the theme:

mkdir /tmp/foo || _Exit $? "mkdir of '/tmp/foo/ failed"

mkdir /tmp/foo && {
	echo "Good: 'mkdir /tmp/foo' worked"
} || {
	echo "BAD: 'mkdir /tmp/foo' failed with: $?"

That also makes a convenient place to remove temp files, lock files, send
messages to syslog, etc.

I disagree here. You want to use a trap for that. 'help trap' but a full discussion is too much typing for now. See "10.6 Trapping Interrupts" in _bash Cookbook_ or just Google it.

=>  git init
=>  git
=>  git remote add
=>  git config refs/heads/master:refs/heads/master
=>  mv repo ../public_html_deploy

The naming (public_html_deploy vs public_html_deploy1) is confusing--the names
are so similar that it's difficult to tell what operations are being
done...and all to easy to rm the wrong dir. I'd suggest changing
"public_html_deploy" to something like "staging" or "git_checkout" or "Fred".

+1. **NAMES MATTER** Can't stress that enough. Clarity matters, can't stress that enough either.

=>  #move untracked files from current webroot into deployment directory tree
=>  cd ../public_html_deploy
=>  cp -r ../public_html/articleImport .
=>  cp -r ../public_html/themes .
=>  cp -r ../public_html/submissions .
=>  cp -r ../public_html/sites/all/themes/  sites/all/themes
=>  cp ../public_html/sites/default/settings.php sites/default/settings.php
=>  cp -r ../public_html/sites/default/files/  sites/default/files

That's too hard for me to parse, given the lack of comments. Will this make
sense to you in 6 months, or to your replacement in a year, when the directory
structure changes?

+1.  Clarity...

=>  #get rid of the stepping stone directory
=>  rm -rf ../public_html_deploy_1

Critical error! You use "public_html_deploy1" when creating the directory and
use "public_html_deploy_1" when removing a different directory.

Meaningful names help *so* much here. The above is easy to screw up. But 'rm -rf ...stage....' and 'rm -rf' will jump out at you as right or wrong.

=>  #set file permission
=>  chmod -R 777 sites/default/files

Why 777?

Yeah, that's almost certainly bad.

This is probably sufficient:

	find sites/default/files -type f -print0 | xargs -0 chmod o+r
	find sites/default/files -type d -print0 | xargs -0 chmod o+rx

and maybe add this, if stictly necessary (but unlikely):
	find sites/default/files -type f -iname "*php" -print0 | xargs -0 chmod o+rx

+1. Note the 'find ... -print0 | xargs -0' that causes both tools use use NULL as the record separator, instead of the default whitespace. In other words, the above will work even with spaces in file/dir names!

There are lots of good books and on-line examples about shell scripting.
Perhaps you should check some of them.

Depending on how you learn:

_Learning the bash Shell_ is the intro, text-book one.
_bash Cookbook_ is the cookbook, "by example" one, with little background, but lots of detail & discussion (I'm co-author). has tons of resources, including a whole of of really good free on-line stuff, if you like that route.

JP Vossen, CISSP            |:::======|
My Account, My Opinions     |=========|
"Microsoft Tax" = the additional hardware & yearly fees for the add-on
software required to protect Windows from its own poorly designed and
implemented self, while the overhead incidentally flattens Moore's Law.
Philadelphia Linux Users Group         --
Announcements -
General Discussion  --