Merge lp://qastaging/~jtv/maas/single-cluster into lp://qastaging/~maas-committers/maas/trunk

[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.

[2]

-run:
+run: build
  bin/django runserver 8000

If you agree with [1] then this needs to depend on build and dev-db.

[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.

[4]

+main() {
+ COMMAND="$1"

COMMAND will end up in the calling environment. The local command
sorts this out:

main() {
    local COMMAND="$1"

Please also let's not pretend we're writing for a plain /bin/sh; let's
just use bash and not go quite as insane. Or, read [6] and use Python,
or just about anything *but* shell.

[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.

[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
}

if [ $# -gt 1 ]
then
    main "$@"
fi

Similar modifications ought to be made throughout the script.

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.

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

Looks good, I think this will work well.

[7]

+main $*

I think this still needs to change to

main "$@"

to avoid whitespace issues.

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

Ah, um, yeah :)