launchpad-reviewers team mailing list archive

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

A few notes from first reading:

2	=== added file 'qa/scripts/prepare-for-juju-with-vdenv'
3	--- qa/scripts/prepare-for-juju-with-vdenv	1970-01-01 00:00:00 +0000
4	+++ qa/scripts/prepare-for-juju-with-vdenv	2012-03-21 18:20:29 +0000

15	+wait_for_next() {

Better comment that, or at least include the next _what_ this waits for.


24	+cat <<'EOF'
25	+
26	+This script assumes that `vdenv` has been started, and that `zimmer` is
27	+running. It will then *destroy* your development/demo database, rebuild
28	+everything, and reconcile with Cobbler.


Did you test this in its current form?  The shell will attempt backtick substitution on this text!  It will try to execute vdenv and zimmer.  Not what you want.

In fact, why did you use backticks at all?  They look weird, and the only reason for using them that I can see is that you really, really, *really* dislike double quotes and as long as you never, ever use them, and manage to avoid all apostrophes, it's usually (but not now) safe to use backticks as quotes inside a string.

That strikes me as a relatively weak reason to use backticks.


51	+cat <<'EOF'
52	+
53	+Next the MAAS server will be started, at which point you can Juju.
54	+
55	+However, once MAAS is running, Juju needs to be pointed at it, and given
56	+credentials with which to manipulate it:
57	+
58	+  1. Copy an OAuth token from <http://localhost:5240/account/prefs/>.
59	+
60	+  2. Interpolate it into the following:
61	+
62	+     {{{
63	+     juju: environments
64	+     environments:
65	+       maas:
66	+         type: maas
67	+         maas-server: 'http://localhost:5240'
68	+         maas-oauth: '${OAUTH_TOKEN_FROM_STEP_1}
69	+         admin-secret: 'nothing'
70	+         juju-origin: lp:maas
71	+     }}}


The shell will attempt variable substitution in this text, so '${OAUTH_TOKEN_FROM_STEP_1}' is likely to come out as the empty string.  Plus, the closing quote is missing.


73	+  3. Customize `juju-origin` as desired.
74	+
75	+  4. Put it into `~/.juju/environments.yaml`.


Executing a few more unexpected commands here.  Not good.


Jeroen
-- 
https://code.launchpad.net/~allenap/maas/pserv-cobbler-reconcile/+merge/98702
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pserv-cobbler-reconcile into lp:maas.