Code review comment for lp://qastaging/~jtv/maas/single-cluster

Thanks for the review. I learned several new things.

> [1]
>
> -build: bin/buildout
> +build: bin/buildout dev-db
>
> I know it's early days, but we probably don't want to create the dev
> database for just a build.

Done.

> [2]
>
> -run:
> +run: build
> bin/django runserver 8000
>
> If you agree with [1] then this needs to depend on build and dev-db.

Done.

> [3]
>
> -harness:
> - . bin/maasdb.sh ; maasdb_init_db db/development disposable
> +harness: dev-db
> bin/django shell
>
> -syncdb:
> +syncdb: dev-db
> bin/django syncdb
>
> However, these don't depend on build, so perhaps run doesn't need to
> either.

It turned out they should have depended on build. Fixed.

> [4]
>
> +main() {
> + COMMAND="$1"
>
> COMMAND will end up in the calling environment. The local command
> sorts this out:

Done, thanks. Wasn't aware of "local."

> [5]
>
> +if test -n "$*"
> +then
> + main $*
> +fi
>
> This won't work if sourced within a script that itself has args, or if
> "set -- $something" has been used in the shell before sourcing this
> script. Because main() defaults to doing nothing if the argument does
> not match I think it's safe to just call main() unconditionally.

Note that this branch eliminates sourcing of the script entirely. I updated comments to remove any suggestion of sourcing it.

> [6]
>
> + case $COMMAND in
> + start) maasdb_init_db $* ;;
> + stop) maasdb_stop_cluster $* ;;
> + query) maasdb_query $* ;;
> + shell) maasdb_shell $* ;;
> + delete-cluster) maasdb_delete_cluster $* ;;
> + esac
> ...
> + main $*
>
> Avoid $* because it rarely does the right thing (in bash at
> least). Also, more quotes needed. The code above behaves badly if
> there are spaces in the arguments passed in. I think the following
> will work without surprises:
>
> main() {
> COMMAND="$1"
> shift
> case "$COMMAND" in
> start) maasdb_init_db "$@" ;;
> stop) maasdb_stop_cluster "$@" ;;
> query) maasdb_query "$@" ;;
> shell) maasdb_shell "$@" ;;
> delete-cluster) maasdb_delete_cluster "$@" ;;
> esac
> }

I wasn't aware of $@ either; reading up on it gave me a much clearer understanding of shell quoting. But since there is no longer any pretense that the functions in the script are for external use, I now shift both the command and the data dir (and fail if they are not given).

> if [ $# -gt 1 ]
> then
> main "$@"
> fi
>
> Similar modifications ought to be made throughout the script.

I don't see any other place where I'd need to make these particular changes.

> To be honest, having to remember crap like this makes me avoid shell
> script these days for anything other than the very most trivial
> things.

You've sold me. This script should not grow; it's just a minimal toolbox.

Jeroen