|
|
Descriptioncmd/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
MessagesTotal messages: 11
Please take a look.
Sign in to reply to this message.
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 cmd/juju-wait/main.go:1: package main so it is "juju-wait ...", not "juju wait ..." ? why not the latter (I mean, why not a regular command like the others)? https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode9 cmd/juju-wait/main.go:9: i don't really get the benefit of this separation, I much prefer all of them to be alphabetically ordered, but as you like. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode25 cmd/juju-wait/main.go:25: would match the regular expression 'error hook failed: "install"'. I'd like to see a couple of lines of actual examples, like: "calling 'juju-wait this and that' does this and that" https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode29 cmd/juju-wait/main.go:29: Juju-wait returns a non-zero exit status if there is an error connecting s/Juju-wait/juju-wait/ - it's a command. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode29 cmd/juju-wait/main.go:29: Juju-wait returns a non-zero exit status if there is an error connecting s/is an/was an/ https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcod... cmd/juju-wait/main.go:112: } so nothing printed when not verbose and not removed? https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go File cmd/juju-wait/main_test.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:34: var waiterTests = []struct { so "pattern" (and the help docs) imply regexp matching, but I cannot see any tests for that - should it have that or it's intentional. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:108: final: "error some info", i'd prefer to have "<status>[:<info>]" as output, to make the distinction between one or the other.
Sign in to reply to this message.
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 it is "juju-wait ...", not "juju wait ..." ? why not the latter (I mean, why > not a regular command like the others)? Because it's an ancillary command, one that might or might not stay around, and will certainly change considerably. I considered not even putting it in juju-core. I don't think it justifies a place as part of the central juju stable; not yet at any rate. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode9 cmd/juju-wait/main.go:9: On 2013/04/12 16:41:16, dimitern wrote: > i don't really get the benefit of this separation, I much prefer all of them to > be alphabetically ordered, but as you like. i haven't decided yet. i usually do it the other way, but i like this way too, so am experimenting with it, if that's ok. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode25 cmd/juju-wait/main.go:25: would match the regular expression 'error hook failed: "install"'. On 2013/04/12 16:41:16, dimitern wrote: > I'd like to see a couple of lines of actual examples, like: "calling 'juju-wait > this and that' does this and that" will do. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcode29 cmd/juju-wait/main.go:29: Juju-wait returns a non-zero exit status if there is an error connecting On 2013/04/12 16:41:16, dimitern wrote: > s/is an/was an/ i think "is" is fine here; we use present tense all through the sentence. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main.go#newcod... cmd/juju-wait/main.go:112: } On 2013/04/12 16:41:16, dimitern wrote: > so nothing printed when not verbose and not removed? that's right - the point of this command is to wait for some desired status, print it and then exit. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go File cmd/juju-wait/main_test.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:34: var waiterTests = []struct { On 2013/04/12 16:41:16, dimitern wrote: > so "pattern" (and the help docs) imply regexp matching, but I cannot see any > tests for that - should it have that or it's intentional. will add a test. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:108: final: "error some info", On 2013/04/12 16:41:16, dimitern wrote: > i'd prefer to have "<status>[:<info>]" as output, to make the distinction > between one or the other. will do.
Sign in to reply to this message.
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 'error hook failed: "install"'. On 2013/04/12 16:51:21, rog wrote: > On 2013/04/12 16:41:16, dimitern wrote: > > I'd like to see a couple of lines of actual examples, like: "calling > 'juju-wait > > this and that' does this and that" > > will do. Done. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go File cmd/juju-wait/main_test.go (right): https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:34: var waiterTests = []struct { On 2013/04/12 16:51:21, rog wrote: > On 2013/04/12 16:41:16, dimitern wrote: > > so "pattern" (and the help docs) imply regexp matching, but I cannot see any > > tests for that - should it have that or it's intentional. > > will add a test. Done. https://codereview.appspot.com/8710043/diff/2001/cmd/juju-wait/main_test.go#n... cmd/juju-wait/main_test.go:108: final: "error some info", On 2013/04/12 16:51:21, rog wrote: > On 2013/04/12 16:41:16, dimitern wrote: > > i'd prefer to have "<status>[:<info>]" as output, to make the distinction > > between one or the other. > > will do. Done.
Sign in to reply to this message.
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 there is an error connecting this is still a command, so "Juju-wait" is not the same as "juju-wait"
Sign in to reply to this message.
Some general comments, overall the code LGTM if there's general agreement that this is the right approach. I appreciate we mostly want something that can fill in what existing scripts require. 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#newcode22 cmd/juju-wait/main.go:22: if there is some status information. I'm not thrilled by this design, it takes basically structured information then requires it be queried in an unstructured manner. Is it to fit in with patterns from jitsu watch or is there other motivation? It also seems to throw away the potential to be smart, knowing for instance that "error" will not transition to "started". This is pushing complexity onto the user, so to write reliable code they need to wait on all possible 'end' states, and get their regexp pattern correct. https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go#newcode40 cmd/juju-wait/main.go:40: var debug = flag.Bool("debug", false, "print debugging messages to standard error") These global flags should really be shared across commands rather than redefined every time, I'm not sure gnuflag makes that easy though. Does Tim's work on cmds help here? 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) + "$") This looks like it would give surprising results. If I wait on "started|error" that would match "pending: no errors yet". Making users anchor stuff themselves isn't ideal, perhaps you should wrap in ^(?:...)$ instead? (Or the spelling of that in go).
Sign in to reply to this message.
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 2013/04/12 23:21:40, gz wrote: > This looks like it would give surprising results. If I wait on "started|error" > that would match "pending: no errors yet". Making users anchor stuff themselves > isn't ideal, perhaps you should wrap in ^(?:...)$ instead? (Or the spelling of > that in go). Luckily, there's no such case. But I see your point.
Sign in to reply to this message.
I'd like to talk about this before you merge it. Please ping me tomorrow morning.
Sign in to reply to this message.
On 2013/04/15 00:04:13, fwereade wrote: > I'd like to talk about this before you merge it. Please ping me tomorrow > morning. As discussed, NOT LGTM for direct inclusion in core -- I'm not comfortable about supporting this interface long-term -- but I accept its utility, and I just want it to go somewhere else. Reasonable?
Sign in to reply to this message.
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 cmd/juju-wait/main.go (right): https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go#newcode22 cmd/juju-wait/main.go:22: if there is some status information. On 2013/04/12 23:21:40, gz wrote: > I'm not thrilled by this design, it takes basically structured information then > requires it be queried in an unstructured manner. Is it to fit in with patterns > from jitsu watch or is there other motivation? it's to make a very simple tool that does what we need now without spending very long wondering what potential smartness might be useful. 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. > > It also seems to throw away the potential to be smart, knowing for instance that > "error" will not transition to "started" 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. > This is pushing complexity onto the > user, so to write reliable code they need to wait on all possible 'end' states, > and get their regexp pattern correct. yup. i don't think it's too hard. https://codereview.appspot.com/8710043/diff/5002/cmd/juju-wait/main.go#newcode40 cmd/juju-wait/main.go:40: var debug = flag.Bool("debug", false, "print debugging messages to standard error") On 2013/04/12 23:21:40, gz wrote: > These global flags should really be shared across commands rather than redefined > every time, I'm not sure gnuflag makes that easy though. Does Tim's work on cmds > help here? maybe. i can't get too fussed about a couple of lines. 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 2013/04/12 23:21:40, gz wrote: > This looks like it would give surprising results. If I wait on "started|error" > that would match "pending: no errors yet". Making users anchor stuff themselves > isn't ideal, perhaps you should wrap in ^(?:...)$ instead? (Or the spelling of > that in go). 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. 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. 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 2013/04/13 07:30:59, dimitern wrote: > On 2013/04/12 23:21:40, gz wrote: > > This looks like it would give surprising results. If I wait on "started|error" > > that would match "pending: no errors yet". Making users anchor stuff > themselves > > isn't ideal, perhaps you should wrap in ^(?:...)$ instead? (Or the spelling of > > that in go). > > Luckily, there's no such case. But I see your point. Sure there is. Go supports that syntax just fine.
Sign in to reply to this message.
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.
|