Add Resolved to the API

Add Resolved to the API



https://code.launchpad.net/~benji/juju-core/1130173/+merge/151555

(do not edit description out of merge proposal)

[benji, 2013-03-04 16:51:09]

Please take a look.

[dimitern, 2013-03-05 22:45:27]

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#newc...
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...
state/statecmd/resolved_test.go:12: //stdtesting "testing"
d

[benji, 2013-03-07 14:33:21]

Thanks for the review.  I have integrated all your review comments.  I had one
or two questions in addition.

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/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?

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/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?

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")
On 2013/03/05 22:45:27, dimitern wrote:
> `unit "wordpress/0" is not in an error state`

Done.

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#newc...
state/statecmd/resolved.go:22: // reexecute previous failed hooks or to continue
as if they had succeeded
On 2013/03/05 22:45:27, dimitern wrote:
> s/reexecute/retry/

Done.

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...
state/statecmd/resolved_test.go:12: //stdtesting "testing"
On 2013/03/05 22:45:27, dimitern wrote:
> d

Good catch.  Subtle comment.

[dimitern, 2013-03-07 15:33:18]

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.

[fwereade, 2013-03-09 14:42:02]

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...
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...
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...
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...
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?

[rog, 2013-03-09 18:01:08]

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...
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.

[benji, 2013-03-12 17:37:24]

I got all of the review comments integrated except for one non-critical one (but
I would like to know what, exactly what it would entail so I can do it in a
future branch).

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/09 18:01:08, rog wrote:
> 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.

Done.

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...
state/statecmd/resolved_test.go:18: repo *charm.LocalRepository
On 2013/03/09 14:42:02, fwereade wrote:
> What are conn and repo for?

This is left over from an earlier revision.  Fixed.

Sidebar: is there a go linter we could use?

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go...
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.

After much spelunking, I can't quite divine what it is you intend for me to do
here.

I can find no instances of comments perpetrating this falsehood and as far as I
can tell, all the Suite calls I see are necessary.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go...
state/statecmd/resolved_test.go:55: c.Assert(err, IsNil)
On 2013/03/09 14:42:02, fwereade wrote:
> ...to here could be replaced with a single call to AddTestingCharm. What am I
> missing?

Cool, I was not aware of AddTestingCharm.  Fixed.

[benji, 2013-03-12 18:45:59]

*** Submitted:

Add Resolved to the API

R=dimitern, fwereade, rog
CC=
https://codereview.appspot.com/7460047