Merge lp:~benji/juju-core/1130173 into lp:~juju/juju-core/trunk

[Benji York]

Add Resolved to the API

https://codereview.appspot.com/7460047/

[Dimiter Naydenov (dimitern)]

Overall LGTM, just some trivials.

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode269
state/api/api_test.go:269: if err != nil && err.Error() == "permission
denied" {
instead of err.Error() == "permission denied", can you use like
ErrPermDenied and compare to that? Or having a helper func
IsPermDenied(taking an error)?

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode276
state/api/api_test.go:276: c.Assert(err, Not(IsNil))
s/Not(IsNil)/NotNil/

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode277
state/api/api_test.go:277: c.Assert(err.Error(), Equals, "unit
\"wordpress/0\" is not in an error state")
`unit "wordpress/0" is not in an error state`

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved.go
File state/statecmd/resolved.go (right):

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved.go#newcode22
state/statecmd/resolved.go:22: // reexecute previous failed hooks or to
continue as if they had succeeded
s/reexecute/retry/

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go
File state/statecmd/resolved_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode12
state/statecmd/resolved_test.go:12: //stdtesting "testing"
d

https://codereview.appspot.com/7460047/

[Dimiter Naydenov (dimitern)]

Thank you for the changes! I answered your questions below.

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode269
state/api/api_test.go:269: if err != nil && err.Error() == "permission
denied" {
On 2013/03/07 14:33:21, benji wrote:
> On 2013/03/05 22:45:27, dimitern wrote:
> > instead of err.Error() == "permission denied", can you use like
ErrPermDenied
> > and compare to that? Or having a helper func IsPermDenied(taking an
error)?

> I'm afraid I don't follow. Are you objecting to the string literal or
something
> else?

Sorry, yes - the literal. It'd be nice to have this as var ErrPermDenied
= fmt.Errorf("permission denied") somewhere

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode276
state/api/api_test.go:276: c.Assert(err, Not(IsNil))
On 2013/03/07 14:33:21, benji wrote:
> On 2013/03/05 22:45:27, dimitern wrote:
> > s/Not(IsNil)/NotNil/

> Done. There are two other instances of Not(Nil) in the tests; shall I
change
> those as well?

Yes please, it's a common shortcut.

https://codereview.appspot.com/7460047/

[William Reade (fwereade)]

Code looks fine, but please fix the tests.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go
File state/statecmd/resolved_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode18
state/statecmd/resolved_test.go:18: repo *charm.LocalRepository
What are conn and repo for?

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode21
state/statecmd/resolved_test.go:21: // Ensure our test suite satisfies
Suite
This is not correct: Suite is a function, and the purpose of the line is
to register the suite for running by gocheck.

The following construction (as snarfed from worker/deployer/simple.go):

var _ Context = (*SimpleContext)(nil)

...is the one that ensures that a pointer to a SimpleContext satisfies
the Context interface (and does so at compile time).

It seems there are several places this misunderstanding is propagated in
this package; please fix those too.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode24
state/statecmd/resolved_test.go:24: func panicWrite(name string, cert,
key []byte) error {
As far as I can tell, the code from here...

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode55
state/statecmd/resolved_test.go:55: c.Assert(err, IsNil)
...to here could be replaced with a single call to AddTestingCharm. What
am I missing?

https://codereview.appspot.com/7460047/

[Roger Peppe (rogpeppe)]

LGTM with the below ErrCode fix and other people's comments addressed.

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/api/api_test.go#newcode269
state/api/api_test.go:269: if err != nil && err.Error() == "permission
denied" {
On 2013/03/07 15:33:18, dimitern wrote:
> On 2013/03/07 14:33:21, benji wrote:
> > On 2013/03/05 22:45:27, dimitern wrote:
> > > instead of err.Error() == "permission denied", can you use like
> ErrPermDenied
> > > and compare to that? Or having a helper func IsPermDenied(taking
an error)?
> >
> > I'm afraid I don't follow. Are you objecting to the string literal
or
> something
> > else?

> Sorry, yes - the literal. It'd be nice to have this as var
ErrPermDenied =
> fmt.Errorf("permission denied") somewhere

actually, this should actually be api.ErrCode(err) ==
api.CodeUnauthorized, i think.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go
File state/statecmd/resolved_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode21
state/statecmd/resolved_test.go:21: // Ensure our test suite satisfies
Suite
On 2013/03/09 14:42:02, fwereade wrote:
> This is not correct: Suite is a function, and the purpose of the line
is to
> register the suite for running by gocheck.

> The following construction (as snarfed from
worker/deployer/simple.go):

> var _ Context = (*SimpleContext)(nil)

> ...is the one that ensures that a pointer to a SimpleContext satisfies
the
> Context interface (and does so at compile time).

> It seems there are several places this misunderstanding is propagated
in this
> package; please fix those too.

yeah, but understandable misunderstanding. the way that Suite returns a
value just so you can use it in this way is highly non-obvious.

https://codereview.appspot.com/7460047/