jujuc client executable

jujuc client executable



https://code.launchpad.net/~fwereade/juju/go-jujuc-client/+merge/103693

Requires: https://code.launchpad.net/~fwereade/juju/go-jujuc-server/+merge/103678

(do not edit description out of merge proposal)

[fwereade, 2012-04-26 13:41:42]

Please take a look.

[rog, 2012-04-27 10:58:23]

looks good.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode13
cmd/jujuc/main.go:13: fmt.Fprintf(os.Stderr, "FATAL: %v\n", err)
s/FATAL/%s: error: /
and insert the command name as mentioned below.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode14
cmd/jujuc/main.go:14: fmt.Fprintf(os.Stderr, server.JUJUC_DOC)
i think the documentation should be here, not in the server.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode18
cmd/jujuc/main.go:18: // requireEnv gets an environment variable or dies.
i think we could just call this "getenv".
the fact that it's local and doesn't return an error should be hint enough that
it can die on failure.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode27
cmd/jujuc/main.go:27: // requireWd gets an absolute path to the working
directory or dies.
i think we could just call this "getwd"

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode43
cmd/jujuc/main.go:43: // arguments. Individual commands should be exposed by
symlinking the command
i don't understand. how does main itself not provide an entry point for testing
with arbitrary command line arguments?
i'd have more faith if Main didn't call os.Exit...

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode45
cmd/jujuc/main.go:45: func Main(args []string) {
will this work if args[0] isn't a bare name?
i think you need to call filepath.Split to find out
the executable name. then you could store that
in a global so that die can print the appropriate
command name.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode57
cmd/jujuc/main.go:57: os.Stdout.Write([]byte(resp.Stdout))
this would be easier if Stdout and Stderr were []byte already.

do we care about losing the correct interleaving of stdout and stderr, BTW?

[fwereade, 2012-04-27 13:44:23]

Please take a look.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode14
cmd/jujuc/main.go:14: fmt.Fprintf(os.Stderr, server.JUJUC_DOC)
On 2012/04/27 10:58:23, rog wrote:
> i think the documentation should be here, not in the server.

Done.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode18
cmd/jujuc/main.go:18: // requireEnv gets an environment variable or dies.
On 2012/04/27 10:58:23, rog wrote:
> i think we could just call this "getenv".
> the fact that it's local and doesn't return an error should be hint enough
that
> it can die on failure.

Done.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode27
cmd/jujuc/main.go:27: // requireWd gets an absolute path to the working
directory or dies.
On 2012/04/27 10:58:23, rog wrote:
> i think we could just call this "getwd"

Done.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode43
cmd/jujuc/main.go:43: // arguments. Individual commands should be exposed by
symlinking the command
On 2012/04/27 10:58:23, rog wrote:
> i don't understand. how does main itself not provide an entry point for
testing
> with arbitrary command line arguments?
> i'd have more faith if Main didn't call os.Exit...

This is for -run-main testing, like all the other main_test tests. Can you think
of a better form of words?

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode45
cmd/jujuc/main.go:45: func Main(args []string) {
On 2012/04/27 10:58:23, rog wrote:
> will this work if args[0] isn't a bare name?
> i think you need to call filepath.Split to find out
> the executable name. then you could store that
> in a global so that die can print the appropriate
> command name.

Huh, could have sworn I'd fixed that. Thanks.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode57
cmd/jujuc/main.go:57: os.Stdout.Write([]byte(resp.Stdout))
On 2012/04/27 10:58:23, rog wrote:
> this would be easier if Stdout and Stderr were []byte already.
> 
> do we care about losing the correct interleaving of stdout and stderr, BTW?

Back in the mists of time, we piped stdout/stderr back to the client, and that
was dropped on the grounds that it was too complex. Happy to revisit the
decision; would prefer to do so in a new branch.

[fwereade, 2012-04-27 13:47:15]

Please take a look.

[rog, 2012-04-27 14:16:30]

LGTM modulo below.

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode43
cmd/jujuc/main.go:43: // arguments. Individual commands should be exposed by
symlinking the command
On 2012/04/27 13:44:23, fwereade wrote:
> On 2012/04/27 10:58:23, rog wrote:
> > i don't understand. how does main itself not provide an entry point for
> testing
> > with arbitrary command line arguments?
> > i'd have more faith if Main didn't call os.Exit...
> 
> This is for -run-main testing, like all the other main_test tests. Can you
think
> of a better form of words?

of course, i'd forgotten.
How about:
// This function is not redundant with main because it
// is exported, and thus can be called by testing code.

I'd put this at the end of the comment BTW.

(although it strikes me that it might be possible for export_test.go to export a
Main that just calls main. perhaps that's evil though!)

https://codereview.appspot.com/6124051/diff/1/cmd/jujuc/main.go#newcode57
cmd/jujuc/main.go:57: os.Stdout.Write([]byte(resp.Stdout))
On 2012/04/27 13:44:23, fwereade wrote:
> On 2012/04/27 10:58:23, rog wrote:
> > this would be easier if Stdout and Stderr were []byte already.
> > 
> > do we care about losing the correct interleaving of stdout and stderr, BTW?
> 
> Back in the mists of time, we piped stdout/stderr back to the client, and that
> was dropped on the grounds that it was too complex. Happy to revisit the
> decision; would prefer to do so in a new branch.

i'm happy with that. just thought i'd mention it.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode16
cmd/jujuc/main.go:16: `[1:]
nice [1:] idiom

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode52
cmd/jujuc/main.go:52: getenv("JUJU_CONTEXT_ID"), getwd(),
filepath.Base(args[0]), args[1:],
sorry i didn't mention this before, but
i'd label these fields.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode64
cmd/jujuc/main.go:64: os.Stdout.Write([]byte(resp.Stdout))
type conversion unnecessary.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go
File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go#newco...
cmd/jujuc/main_test.go:44: var expectHelp = `The jujuc command forwards
invocations over RPC for execution by the juju
i don't think you need to duplicate the entire error message here - it just
means it has to be changed in two places each time. how about making it a
minimal regexp ("The jujuc command.*\n"?) and using Matches instead of Equals?

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go#newco...
cmd/jujuc/main_test.go:158: err := fmt.Sprintf("error: dial unix %s: no such
file or directory\n", badSock)
i'm not sure the ENOENT is always guaranteed to produce that error message. how
about a regexp "error: dial unix %s: .*"?

[fwereade, 2012-04-27 16:02:41]

Please take a look.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode16
cmd/jujuc/main.go:16: `[1:]
On 2012/04/27 14:16:31, rog wrote:
> nice [1:] idiom

I wasn't sure if it was nice or evil :)

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode52
cmd/jujuc/main.go:52: getenv("JUJU_CONTEXT_ID"), getwd(),
filepath.Base(args[0]), args[1:],
On 2012/04/27 14:16:31, rog wrote:
> sorry i didn't mention this before, but
> i'd label these fields.

Done.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main.go#newcode64
cmd/jujuc/main.go:64: os.Stdout.Write([]byte(resp.Stdout))
On 2012/04/27 14:16:31, rog wrote:
> type conversion unnecessary.

Done.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go
File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go#newco...
cmd/jujuc/main_test.go:44: var expectHelp = `The jujuc command forwards
invocations over RPC for execution by the juju
On 2012/04/27 14:16:31, rog wrote:
> i don't think you need to duplicate the entire error message here - it just
> means it has to be changed in two places each time. how about making it a
> minimal regexp ("The jujuc command.*\n"?) and using Matches instead of Equals?

I'm most inclined to export it and use it directly, now I think of it.

https://codereview.appspot.com/6124051/diff/8001/cmd/jujuc/main_test.go#newco...
cmd/jujuc/main_test.go:158: err := fmt.Sprintf("error: dial unix %s: no such
file or directory\n", badSock)
On 2012/04/27 14:16:31, rog wrote:
> i'm not sure the ENOENT is always guaranteed to produce that error message.
how
> about a regexp "error: dial unix %s: .*"?

Done.

[fwereade, 2012-04-30 01:01:30]

Please take a look.

[rog, 2012-04-30 07:40:15]

On 2012/04/30 01:01:30, fwereade wrote:
> Please take a look.

LGTM

[niemeyer, 2012-05-02 23:21:51]

Thanks for keeping this so simple. I have a minor request if that's ok:

https://codereview.appspot.com/6124051/diff/12001/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/12001/cmd/jujuc/main.go#newcode19
cmd/jujuc/main.go:19: func die(err error) {
Hmm.. die calls Exit from within a stack of functions that give no hint that
they're silently crashing hard. Instead of doing that, it'd be much nicer to
have main() defined as:

func main() {
    err := Main(os.Args)
    if err != nil {
        fmt.Fprintf(os.Stderr, "error: %v")
        os.Exit(1)
    }
}

And simply return an error as usual when problems happen.

[fwereade, 2012-05-03 08:38:43]

On 2012/05/02 23:21:51, niemeyer wrote:
> Thanks for keeping this so simple. I have a minor request if that's ok:
> 
> https://codereview.appspot.com/6124051/diff/12001/cmd/jujuc/main.go
> File cmd/jujuc/main.go (right):
> 
> https://codereview.appspot.com/6124051/diff/12001/cmd/jujuc/main.go#newcode19
> cmd/jujuc/main.go:19: func die(err error) {
> Hmm.. die calls Exit from within a stack of functions that give no hint that
> they're silently crashing hard. Instead of doing that, it'd be much nicer to
> have main() defined as:
> 
> func main() {
>     err := Main(os.Args)
>     if err != nil {
>         fmt.Fprintf(os.Stderr, "error: %v")
>         os.Exit(1)
>     }
> }
> 
> And simply return an error as usual when problems happen.

OK, I'd hoped this was compact enough that it'd be clear anyway, but I'll see
how it looks :).

[fwereade, 2012-05-03 08:46:38]

On 2012/05/03 08:38:43, fwereade wrote:
> OK, I'd hoped this was compact enough that it'd be clear anyway, but I'll see
> how it looks :).

Hmm; I don't want logic in main(), because I can't test it; and having main
calling Main calling yet-something-else really feels a bit painful. How's this
for a compromise? (forthcoming)

[fwereade, 2012-05-03 08:47:41]

Please take a look.

[fwereade, 2012-05-03 09:55:26]

On 2012/05/03 08:47:41, fwereade wrote:
> Please take a look.

(oh, blast, ignore this for now; prereq merge)

[fwereade, 2012-05-03 10:20:23]

Please take a look.

[niemeyer, 2012-05-03 15:58:43]

On 2012/05/03 08:46:38, fwereade wrote:
> Hmm; I don't want logic in main(), because I can't test it; and having main
> calling Main calling yet-something-else really feels a bit painful. How's this
> for a compromise? (forthcoming)

I'd still prefer the proposed main version. I'm happy to have the compromise of
not testing it, in exchange for not having os.Exit anywhere else, and using
exactly the same "return err" convention universally.

[fwereade, 2012-05-03 16:39:53]

Please take a look.

[niemeyer, 2012-05-03 17:30:52]

I thought we had agreed that the idea was to return the error value from Main.
This still looks simpler to me:

LGTM assuming it.

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go
File cmd/jujuc/main.go (right):

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode49
cmd/jujuc/main.go:49: func Main(args []string) int {
func Main(args []string) (code int, err error) {

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode53
cmd/jujuc/main.go:53: return 2
return 2, fmt.Errorf("jujuc is not supposed to be called directly")

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode54
cmd/jujuc/main.go:54: }
code = 1

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode57
cmd/jujuc/main.go:57: return exitCode(err)
return

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode61
cmd/jujuc/main.go:61: return exitCode(err)
return

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode71
cmd/jujuc/main.go:71: return exitCode(err)
return

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode75
cmd/jujuc/main.go:75: return exitCode(err)
return

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode81
cmd/jujuc/main.go:81: return exitCode(err)
return

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode85
cmd/jujuc/main.go:85: return resp.Code
return resp.Code, nil

https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode89
cmd/jujuc/main.go:89: os.Exit(Main(os.Args))
code, err := Main(os.Args)
if err != nil {
    fmt.Fprintf(os.Stderr, "error: %v\n", err)
}
os.Exit(code)

[fwereade, 2012-05-03 19:54:38]

On 2012/05/03 17:30:52, niemeyer wrote:
> I thought we had agreed that the idea was to return the error value from Main.
> This still looks simpler to me:
> 
> LGTM assuming it.
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go
> File cmd/jujuc/main.go (right):
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode49
> cmd/jujuc/main.go:49: func Main(args []string) int {
> func Main(args []string) (code int, err error) {
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode53
> cmd/jujuc/main.go:53: return 2
> return 2, fmt.Errorf("jujuc is not supposed to be called directly")
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode54
> cmd/jujuc/main.go:54: }
> code = 1
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode57
> cmd/jujuc/main.go:57: return exitCode(err)
> return
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode61
> cmd/jujuc/main.go:61: return exitCode(err)
> return
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode71
> cmd/jujuc/main.go:71: return exitCode(err)
> return
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode75
> cmd/jujuc/main.go:75: return exitCode(err)
> return
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode81
> cmd/jujuc/main.go:81: return exitCode(err)
> return
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode85
> cmd/jujuc/main.go:85: return resp.Code
> return resp.Code, nil
> 
> https://codereview.appspot.com/6124051/diff/17002/cmd/jujuc/main.go#newcode89
> cmd/jujuc/main.go:89: os.Exit(Main(os.Args))
> code, err := Main(os.Args)
> if err != nil {
>     fmt.Fprintf(os.Stderr, "error: %v\n", err)
> }
> os.Exit(code)

Ah, I hadn't picked up on the code+err return -- sorry, I think that might be
why it seemed we were talking past one another a bit. That's nice; thank you.

[fwereade, 2012-05-03 21:02:10]

*** Submitted:

jujuc client executable

R=rog, niemeyer
CC=
https://codereview.appspot.com/6124051