Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(228)

Issue 8710043: cmd/juju-wait: new command

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by rog
Modified:
10 years, 11 months ago
Reviewers:
dimitern, mp+158635, fwereade, gz
Visibility:
Public.

Description

cmd/juju-wait: new command A fairly minimal implementation. Since this isn't an important piece of infrastructure, time is tight, and in keeping with other non-essential commands, the testing is pretty light. https://code.launchpad.net/~rogpeppe/juju-core/284-juju-wait/+merge/158635 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : cmd/juju-wait: new command #

Total comments: 18

Patch Set 3 : cmd/juju-wait: new command #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+335 lines, -0 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A cmd/juju-wait/main.go View 1 2 1 chunk +154 lines, -0 lines 9 comments Download
A cmd/juju-wait/main_test.go View 1 2 1 chunk +179 lines, -0 lines 0 comments Download

Messages

Total messages: 11
rog
Please take a look.
10 years, 11 months ago (2013-04-12 14:46:20 UTC) #1
dimitern
Looks good so far, but I have a few questions. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go File cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode1 ...
10 years, 11 months ago (2013-04-12 16:41:16 UTC) #2
rog
https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go File cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode1 cmd/juju-wait/main.go:1: package main On 2013/04/12 16:41:16, dimitern wrote: > so ...
10 years, 11 months ago (2013-04-12 16:51:21 UTC) #3
rog
Please take a look. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go File cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode25 cmd/juju-wait/main.go:25: would match the regular expression ...
10 years, 11 months ago (2013-04-12 17:48:13 UTC) #4
dimitern
LGTM, thanks. https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go File cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go#newcode29 cmd/juju-wait/main.go:29: Juju-wait returns a non-zero exit status if ...
10 years, 11 months ago (2013-04-12 17:57:14 UTC) #5
gz
Some general comments, overall the code LGTM if there's general agreement that this is the ...
10 years, 11 months ago (2013-04-12 23:21:40 UTC) #6
dimitern
https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go File cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go#newcode54 cmd/juju-wait/main.go:54: statusPattern, err := regexp.Compile("^" + flag.Arg(1) + "$") On ...
10 years, 11 months ago (2013-04-13 07:30:59 UTC) #7
fwereade
I'd like to talk about this before you merge it. Please ping me tomorrow morning.
10 years, 11 months ago (2013-04-15 00:04:13 UTC) #8
fwereade
On 2013/04/15 00:04:13, fwereade wrote: > I'd like to talk about this before you merge ...
10 years, 11 months ago (2013-04-15 11:04:18 UTC) #9
rog
As agreed on-line, I've moved this to another project. Please review there. https://codereview.appspot.com/8565044/ https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go File ...
10 years, 11 months ago (2013-04-15 18:03:45 UTC) #10
gz
10 years, 11 months ago (2013-04-15 18:37:32 UTC) #11
On 2013/04/15 18:03:45, rog wrote:
> As agreed on-line, I've moved this to another project.
> Please review there.
> 
> https://codereview.appspot.com/8565044/

Thanks, will comment there shortly.

> there are hundreds of ways to be "smart" (jitsu-watch shows
> some possibilities) but for the moment all we want is a tool
> that is sufficient for the charm testing scripts.
> 
> i hope this is.

With William's suggestion to move this out of juju-core, I'm happy with this
being an interim solution.

> that's an example of unjustified smartness IMHO. an error
> state can very easily transition to "started" if we call
> resolved on it. someone might well want to test that behaviour
> and i don't want to make it harder for them to do so.

Right, though that is a manual step. My point is we want the common stuff the
user wants to do easy, without them having to consider all of these complexities
just to wait for a machine to come up.


> maybe. i can't get too fussed about a couple of lines.

Okay. This is the attitude that got us code copied all over the place though,
the odd few lines adds up after hundreds of commits.


> I'm not entirely sure where your remark is coming from.
> The "^" and "$" in the above line is intended to anchor the
> regexp (if you wait for "started|error", it will not match "pending: no errors
> yet").
> 
> That said, thanks for pointing to this line, as I have made a mistake by not
> placing the expression in a group
> ("started|error" *will* match "pending: no error", oops!)
> 
> I've fixed that issue.

...okay, so that's fixed. But I have trouble following your response here
though, the first paragraph says you don't understand my comment, the second
demonstrates that you do.

> Also I'm not sure why you're using a non-capturing group there (I had to look
up
> the syntax), as we're not deriving any subexpressions from the regexp.

Just habit, I use non-capturing unless I actually want to capture.

> > Luckily, there's no such case. But I see your point.
> 
> Sure there is. Go supports that syntax just fine.

I think Dimiter's comment was about there not being any such silly message as
"pending: no errors yet" so that particular example isn't something that would
fail now.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b