Code review comment for lp://staging/~danilo/juju-core/bootstrap-verify

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Almost there and nice! Some minor suggestions, but big +1 on fwereade's
comments about changing jujutest.

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go#newcode440
environs/dummy/environs.go:440: return fmt.Errorf("Provider storage is
not writeable.")
s/Provider/provider/ and no dot at the end.

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go#newcode268
environs/ec2/ec2.go:268: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/04 12:42:52, mue wrote:
> Lowercase, "provider storage ...", and not dot at the end.

+1

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go#newcode117
environs/maas/environ.go:117: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/04 12:42:52, mue wrote:
> Dito, lowercase and dot.

+1

https://codereview.appspot.com/9876043/diff/4001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/openstack/provider.go#newcode494
environs/openstack/provider.go:494: return fmt.Errorf("Provider storage
is not writeable.")
On 2013/06/04 12:42:52, mue wrote:
> And again lowercase and dot.

+1

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode18
environs/storage.go:18: const VERIFICATION_FILENAME string =
"bootstrap-verify"
Please, this is not C :)
const VerificationFilename string = "bootstrap-verify"

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode19
environs/storage.go:19: const VERIFICATION_CONTENT = "juju-core storage
writing verified: ok"
ditto: VerificationContent

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go
File environs/storage_test.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#newcode36
environs/storage_test.go:36: type MockStorage struct {
does it have to be exported?

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#newcode44
environs/storage_test.go:44: "Put('%s', '%s', %d)", filename, content,
length)
if you're doing the call multiline, please:
log_message := fmt.Sprintf(
   "...",
   filename,
   content,
   length,
)

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#newcode67
environs/storage_test.go:67: type VerifyStorageSuite struct{}
again, the suite doesn't need to be exported (verifyStorageSuite or even
just storageSuite)

https://codereview.appspot.com/9876043/

« Back to merge proposal