Merge lp://staging/~niemeyer/juju-core/new-state-builddb into lp://staging/~juju/juju-core/trunk

Reviewers: mp+126537_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/builddb: tweaks for new state

This also moves it from util/ to cmd/, which is fine now that
environs is building tools explicitly by their name when
shipping to an environment. util/ is gone.

https://code.launchpad.net/~niemeyer/juju-core/new-state-builddb/+merge/126537

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/builddb/main.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: <email address hidden>
+New revision: <email address hidden>

Index: cmd/builddb/main.go
=== modified file 'cmd/builddb/main.go'
--- util/builddb/main.go 2012-09-21 14:08:45 +0000
+++ cmd/builddb/main.go 2012-09-26 20:14:53 +0000
@@ -53,18 +53,26 @@
    return err
   }

- log.Printf("Waiting for unit to reach started status...")
+ log.Printf("Waiting for unit to reach %q status...", state.UnitStarted)
   unit := units[0]
- status, _, err := unit.Status()
+ last, info, err := unit.Status()
   if err != nil {
    return err
   }
- for status != state.UnitStarted {
+ logStatus(last, info)
+ for last != state.UnitStarted {
    time.Sleep(2 * time.Second)
- status, _, err = unit.Status()
+ if err := unit.Refresh(); err != nil {
+ return err
+ }
+ status, info, err := unit.Status()
    if err != nil {
     return err
    }
+ if status != last {
+ logStatus(status, info)
+ last = status
+ }
   }
   addr, err := unit.PublicAddress()
   if err != nil {
@@ -74,3 +82,11 @@
   log.Printf("Remember to destroy the environment when you're done...")
   return nil
  }
+
+func logStatus(status state.UnitStatus, info string) {
+ if info == "" {
+ log.Printf("Unit status is %q", status)
+ } else {
+ log.Printf("Unit status is %q: %s", status, info)
+ }
+}

LGTM

(Hmm, I'm really starting to feel that it was a bad idea to made Status
depend on unit liveness... they should be separated, and we should fix
the status command so that it's responsible for showing dead agents;
then you could use an actual unit watcher instead of polling.)

https://codereview.appspot.com/6567054/

On 2012/09/26 20:31:53, fwereade wrote:
> LGTM

> (Hmm, I'm really starting to feel that it was a bad idea to made
Status depend
> on unit liveness... they should be separated, and we should fix the
status
> command so that it's responsible for showing dead agents; then you
could use an
> actual unit watcher instead of polling.)

+1 on separating somehow.

For watching, I actually think we need a sort of unit.Wait(timeout)
method, so that we can use it from a "juju wait" command.

https://codereview.appspot.com/6567054/

On 2012/09/26 23:22:06, niemeyer wrote:
> On 2012/09/26 20:31:53, fwereade wrote:
> > LGTM
> >
> > (Hmm, I'm really starting to feel that it was a bad idea to made
Status depend
> > on unit liveness... they should be separated, and we should fix the
status
> > command so that it's responsible for showing dead agents; then you
could use
> an
> > actual unit watcher instead of polling.)

> +1 on separating somehow.

> For watching, I actually think we need a sort of unit.Wait(timeout)
method, so
> that we can use it from a "juju wait" command.

Perhaps more properly:

      info, err := unit.Wait(status, timeout)

Built in terms of the unit watcher.

https://codereview.appspot.com/6567054/

*** Submitted:

cmd/builddb: tweaks for new state

This also moves it from util/ to cmd/, which is fine now that
environs is building tools explicitly by their name when
shipping to an environment. util/ is gone.

R=fwereade
CC=
https://codereview.appspot.com/6567054

https://codereview.appspot.com/6567054/