Merge lp:~axwalk/juju-core/lp1246983-cli-api-resolved into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/juju: use API for resolved

Partially fixes #1246983

https://codereview.appspot.com/20670044/

cmd/juju: use API for resolved

Partially fixes #1246983

https://codereview.appspot.com/20670044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+193564_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: use API for resolved

Partially fixes #1246983

https://code.launchpad.net/~axwalk/juju-core/lp1246983-cli-api-resolved/+merge/193564

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/20670044/

Affected files (+8, -10 lines):
   [revision details]
   cmd/juju/resolved.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131031053536-8t30819l7xwprtxb
+New revision: <email address hidden>

Index: cmd/juju/resolved.go
=== modified file 'cmd/juju/resolved.go'
--- cmd/juju/resolved.go 2013-08-13 19:07:35 +0000
+++ cmd/juju/resolved.go 2013-11-01 09:26:22 +0000
@@ -48,14 +48,10 @@
  }

  func (c *ResolvedCommand) Run(_ *cmd.Context) error {
- conn, err := juju.NewConnFromName(c.EnvName)
- if err != nil {
- return err
- }
- defer conn.Close()
- unit, err := conn.State.Unit(c.UnitName)
- if err != nil {
- return err
- }
- return unit.Resolve(c.Retry)
+ client, err := juju.NewAPIClientFromName(c.EnvName)
+ if err != nil {
+ return err
+ }
+ defer client.Close()
+ return client.Resolved(c.UnitName, c.Retry)
  }

[John A Meinel (jameinel)]

This code is nice and straightforward, but because I was digging around
with the other test changes, I noticed that the tests of the API don't
actually exercise the Retry logic.

I'm not going to block landing this on writing those tests, because you
aren't *reducing* test coverage at all. But could you try to add some
coverage while we're away from it?

If the test is too fiddly or time consuming, file a tech-debt bug, link
to it in the source code and you can land this.

https://codereview.appspot.com/20670044/

[Andrew Wilkins (axwalk)]

On 2013/11/02 20:30:56, john2 wrote:
> This code is nice and straightforward, but because I was digging
around with the
> other test changes, I noticed that the tests of the API don't actually
exercise
> the Retry logic.

> I'm not going to block landing this on writing those tests, because
you aren't
> *reducing* test coverage at all. But could you try to add some
coverage while
> we're away from it?

> If the test is too fiddly or time consuming, file a tech-debt bug,
link to it in
> the source code and you can land this.

I've expanded TestClientUnitResolved to ensure that if retry is true,
the correct resolved mode is set.

https://codereview.appspot.com/20670044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/20670044/

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/20670044/diff/20001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/20670044/diff/20001/state/apiserver/client/client_test.go#newcode375
state/apiserver/client/client_test.go:375: }}
I would actually say that this is a case where having 2 tests is clearer
about what is going on. Possibly a helper function that takes the
parameters if there is too much repeated code.

But meh, good enough.

https://codereview.appspot.com/20670044/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/20670044/diff/20001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/20670044/diff/20001/state/apiserver/client/client_test.go#newcode375
state/apiserver/client/client_test.go:375: }}
On 2013/11/05 04:43:13, jameinel wrote:
> I would actually say that this is a case where having 2 tests is
clearer about
> what is going on. Possibly a helper function that takes the parameters
if there
> is too much repeated code.

Agreed; changing to two tests.

> But meh, good enough.

https://codereview.appspot.com/20670044/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in state/apiserver/client/client_test.go

=== modified file 'cmd/juju/resolved.go'
--- cmd/juju/resolved.go	2013-08-13 19:07:35 +0000
+++ cmd/juju/resolved.go	2013-11-05 05:12:50 +0000
@@ -48,14 +48,10 @@
 }
 
 func (c *ResolvedCommand) Run(_ *cmd.Context) error {
-	conn, err := juju.NewConnFromName(c.EnvName)
-	if err != nil {
-		return err
-	}
-	defer conn.Close()
-	unit, err := conn.State.Unit(c.UnitName)
-	if err != nil {
-		return err
-	}
-	return unit.Resolve(c.Retry)
+	client, err := juju.NewAPIClientFromName(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+	return client.Resolved(c.UnitName, c.Retry)
 }

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2013-11-05 04:13:50 +0000
+++ state/apiserver/client/client_test.go	2013-11-05 05:12:50 +0000
@@ -448,7 +448,7 @@
 	}
 }
 
-func (s *clientSuite) TestClientUnitResolved(c *gc.C) {
+func (s *clientSuite) testClientUnitResolved(c *gc.C, retry bool, expectedResolvedMode state.ResolvedMode) {
 	// Setup:
 	s.setUpScenario(c)
 	u, err := s.State.Unit("wordpress/0")
@@ -456,7 +456,7 @@
 	err = u.SetStatus(params.StatusError, "gaaah", nil)
 	c.Assert(err, gc.IsNil)
 	// Code under test:
-	err = s.APIState.Client().Resolved("wordpress/0", false)
+	err = s.APIState.Client().Resolved("wordpress/0", retry)
 	c.Assert(err, gc.IsNil)
 	// Freshen the unit's state.
 	err = u.Refresh()
@@ -464,7 +464,15 @@
 	// And now the actual test assertions: we set the unit as resolved via
 	// the API so it should have a resolved mode set.
 	mode := u.Resolved()
-	c.Assert(mode, gc.Equals, state.ResolvedNoHooks)
+	c.Assert(mode, gc.Equals, expectedResolvedMode)
+}
+
+func (s *clientSuite) TestClientUnitResolved(c *gc.C) {
+	s.testClientUnitResolved(c, false, state.ResolvedNoHooks)
+}
+
+func (s *clientSuite) TestClientUnitResolvedRetry(c *gc.C) {
+	s.testClientUnitResolved(c, true, state.ResolvedRetryHooks)
 }
 
 func (s *clientSuite) TestClientServiceDeployCharmErrors(c *gc.C) {