maas-devel team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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
-- 
https://code.launchpad.net/~jtv/maas/single-cluster/+merge/88739
Your team MaaS Developers is subscribed to branch lp:maas.