Code review comment for lp:~axwalk/juju-core/manual-bootstrap
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Looks good. Some minor comments and suggestions below.
https:/
File cmd/juju/
https:/
cmd/juju/
bootstrapStorage}
nice
https:/
File environs/
https:/
environs/
that returns a environs.Storage that may
I think I'd define this inside environs/
This package doesn't define or use it, and that file
is a useful reference for people implementing providers.
https:/
File environs/
https:/
environs/
(environs.Storage, error) {
Doc comment please.
Also, wouldn't it make more sense to define NewFileStorageR
terms of NewFileStorage? That way you wouldn't need the dynamic type
conversion, as Storage can be assigned to StorageReader.
For future consideration, I think filestorage.New and
filestorage.
filestorage.
https:/
File environs/
https:/
environs/
s/machineenv/
https:/
File environs/
https:/
environs/
argument is empty")
Errors don't generally start with a capital letter.
Also, if it's only used once, I wouldn't bother defining a variable for
it.
https:/
environs/
argument is nil")
ditto
https:/
environs/
s/will/will be/ ?
https:/
environs/
provisioned: %v", err)
"failed to check provisioned status: %v" ?
https:/
environs/
Why not defer this after the call to SaveState and thereby lose the need
for the savedState variable?
https:/
environs/
hardware characteristics: %v", err)
s/err = /return /
https:/
environs/
s/machineenv/
https:/
File environs/
https:/
environs/
/etc/init/ | grep juju.*\\.conf || exit 0"
How about a more direct test?
"ls /etc/init/
which will succeed if provisioned and fail otherwise.
https:/
environs/
"bash")
or
cmd := exec.Command("ssh", sshHost, "bash", "-c",
checkProvisione
?
https:/
environs/
bytes.NewBuffer
strings.NewReader would seem more appropriate if you continue to use
Stdin.
https:/
environs/
I think you want Output not CombinedOutput here.
https:/
environs/
I wonder if this should include a "set -e"
https:/
File environs/
https:/
environs/
[]int{255, 0} {
Please add a log statement here, so that if the test fails, we
know which iteration we were on.
Something like this is conventional:
c.Logf("test %d: code %d", i, errorCode)
https:/
File environs/
https:/
environs/
output string, rc int) func() {
I found these names quite confusing; the respond method doesn't
respond to anything...
How about something like "installFakeSSH" instead instead of
sshresponse?
Then the type could be fakeSSH, and the respond method could be named
SetUp or similar.
https:/
environs/
*gc.C) func() {
I'd like to see a doc comment on this please.
https:/
environs/
output, rc)
I think things can be simpler here. How about something like:
var restores := []func()
add := func(input, output string, rc int) {
restore := sshresponse(c, input, output, rc)
restores = append(restores, restore)
}
if !r.skipProvisio
add("", "", r.provisionAgen
}
add(detectionSc
add(checkProvis
return func() {
for i := len(restores)-1; i >= 0; i-- {
}
}
alternatively, you could use jc.Restorer throughout, and
add a method to it:
// Add returns a Restorer that restores first f1
// and then f. It is valid to call this on a nil
// Restorer.
func (f Restorer) Add(f1 Restorer) Restorer {
return func() {
if f != nil {
}
}
}
then you could do:
var restore jc.Restorer
add := func(input, output string, rc int) {
restore = restore.
}
if !r.skipProvisio
add("", "", r.provisionAgen
}
add(detectionSc
add(checkProvis
return restore
On balance I prefer the latter, because the defer
calls can be:
defer sshresponder{
which is more obvious to my eyes (I had missed the extra set of () in
the call).
https:/