Merge lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1555
Proposed branch: lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 202 lines (+171/-0)
4 files modified
cmd/juju/helptool.go (+104/-0)
cmd/juju/helptool_test.go (+63/-0)
cmd/juju/main.go (+3/-0)
cmd/juju/main_test.go (+1/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+176611@code.launchpad.net

Commit message

cmd/juju: add help-tool subcommand

This change introduces a new subommand to juju:
    juju help-tool: list all tools a la "help commands"
    juju help-tool <tool>: show tool's help

https://codereview.appspot.com/11758043/

Description of the change

cmd/juju: add help-tool subcommand

This change introduces a new subommand to juju:
    juju help-tool: list all tools a la "help commands"
    juju help-tool <tool>: show tool's help

https://codereview.appspot.com/11758043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+176611_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/jujud: add help-tool subcommand

This change introduces a new subommand to jujud:
     jujud help-tool: list all tools a la "help commands"
     jujud help-tool <tool>: show tool's help

Note that this is cmd/jujud, not cmd/juju.

https://code.launchpad.net/~axwalk/juju-core/lp1186969-add-jujud-tools-help/+merge/176611

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A cmd/jujud/helptool.go
   M cmd/jujud/main.go

Revision history for this message
Roger Peppe (rogpeppe) wrote :

This looks reasonable, but I'm not convinced by it living in jujud. The
jujud command is really an implementation detail and there's no
particular reason that it will be installed on a user's machine, which
is probably where charms are being written. Importing jujuc into juju
grows the binary by about 3%, but I don't think that's a big issue.

I'd also like to see a test.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go
File cmd/jujud/helptool.go (right):

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode20
cmd/jujud/helptool.go:20: func (_ dummyHookContext) UnitName() string {
s/_ //

and below

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode79
cmd/jujud/helptool.go:79: cmds := make([]cmd.Command, len(names))
How about:
  cmds := make([]cmd.Command, 0, len(names))

then append to it inside the next loop rather than assigning
to a known index? Then you won't need the "if cmds[i] != nil"
condition inside the print loop.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode89
cmd/jujud/helptool.go:89: const lineFormat = "%-*s %s\n"
I don't think there's a benefit from separating the format from the
Fprintf here (it conceals it from govet)

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode90
cmd/jujud/helptool.go:90: const outputFormat = "%s"
d (I'm quite surprised the compiler didn't complain about this being
unused actually)

https://codereview.appspot.com/11758043/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2013/07/24 08:50:11, rog wrote:
> This looks reasonable, but I'm not convinced by it living in jujud.
The jujud
> command is really an implementation detail and there's no particular
reason that
> it will be installed on a user's machine, which is probably where
charms are
> being written. Importing jujuc into juju grows the binary by about 3%,
but I
> don't think that's a big issue.

Is it possible to have a jujud on the target system that is out of sync
with the juju client, though? The other reason that I put it in jujud
was to avoid misleading help. But I guess that's being a bit pedantic,
and probably not going to be an issue. I'll move it.

> I'd also like to see a test.

Fair enough, I'll add one.

> https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go
> File cmd/jujud/helptool.go (right):

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode20
> cmd/jujud/helptool.go:20: func (_ dummyHookContext) UnitName() string
{
> s/_ //

> and below

Done.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode79
> cmd/jujud/helptool.go:79: cmds := make([]cmd.Command, len(names))
> How about:
> cmds := make([]cmd.Command, 0, len(names))

> then append to it inside the next loop rather than assigning
> to a known index? Then you won't need the "if cmds[i] != nil"
> condition inside the print loop.

Actually that's just a brainfart; it should be make([]cmd.Command,
len(names)).
The condition will need to remain: it's there because NewCommand might
fail
in the first loop.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode89
> cmd/jujud/helptool.go:89: const lineFormat = "%-*s %s\n"
> I don't think there's a benefit from separating the format from the
Fprintf here
> (it conceals it from govet)

Done.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode90
> cmd/jujud/helptool.go:90: const outputFormat = "%s"
> d (I'm quite surprised the compiler didn't complain about this being
unused
> actually)

Oops - removed.

https://codereview.appspot.com/11758043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 24 July 2013 10:54, Andrew Wilkins <email address hidden> wrote:
> https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode79
>> cmd/jujud/helptool.go:79: cmds := make([]cmd.Command, len(names))
>> How about:
>> cmds := make([]cmd.Command, 0, len(names))
>
>> then append to it inside the next loop rather than assigning
>> to a known index? Then you won't need the "if cmds[i] != nil"
>> condition inside the print loop.
>
> Actually that's just a brainfart; it should be make([]cmd.Command,
> len(names)).
> The condition will need to remain: it's there because NewCommand might
> fail
> in the first loop.

I'm suggesting that we only fill the slice with non-nil elements
(hence my suggestion to use append rather than "cmds[i] = c")
which would mean that even if NewCommand fails, we won't
need to test for a nil element in the second loop.

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :

> I'm suggesting that we only fill the slice with non-nil elements
> (hence my suggestion to use append rather than "cmds[i] = c")
> which would mean that even if NewCommand fails, we won't
> need to test for a nil element in the second loop.

Done, PTAL.

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 2013/07/25 01:25:25, axw wrote:
> Please take a look.

LGTM thanks!

https://codereview.appspot.com/11758043/

Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (6.4 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 2.259s
ok launchpad.net/juju-core/agent/tools 21.264s
ok launchpad.net/juju-core/bzr 6.840s
ok launchpad.net/juju-core/cert 3.141s
ok launchpad.net/juju-core/charm 0.543s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.009s
ok launchpad.net/juju-core/cmd 0.348s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
FAIL: main_test.go:256: MainSuite.TestHelpCommands

main_test.go:271:
    // The names should be output in alphabetical order, so don't sort.
    c.Assert(names, DeepEquals, commandNames)
... obtained []string = []string{"add-machine", "add-relation", "add-unit", "bootstrap", "debug-log", "deploy", "destroy-environment", "destroy-machine", "destroy-relation", "destroy-service", "destroy-unit", "env", "expose", "generate-config", "get", "get-constraints", "get-env", "get-environment", "help", "help-tool", "image-metadata", "init", "publish", "remove-relation", "remove-unit", "resolved", "scp", "set", "set-constraints", "set-env", "set-environment", "ssh", "stat", "status", "switch", "sync-tools", "terminate-machine", "unexpose", "upgrade-charm", "upgrade-juju", "version"}
... expected []string = []string{"add-machine", "add-relation", "add-unit", "bootstrap", "debug-log", "deploy", "destroy-environment", "destroy-machine", "destroy-relation", "destroy-service", "destroy-unit", "env", "expose", "generate-config", "get", "get-constraints", "get-env", "get-environment", "help", "image-metadata", "init", "publish", "remove-relation", "remove-unit", "resolved", "scp", "set", "set-constraints", "set-env", "set-environment", "ssh", "stat", "status", "switch", "sync-tools", "terminate-machine", "unexpose", "upgrade-charm", "upgrade-juju", "version"}

OOPS: 165 passed, 1 FAILED
--- FAIL: TestPackage (102.42 seconds)
FAIL
FAIL launchpad.net/juju-core/cmd/juju 102.730s
ok launchpad.net/juju-core/cmd/jujud 40.914s
ok launchpad.net/juju-core/constraints 0.015s
ok launchpad.net/juju-core/container/lxc 0.314s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.298s
ok launchpad.net/juju-core/environs 1.798s
? launchpad.net/juju-core/environs/all [no test files]
ok launchpad.net/juju-core/environs/azure 3.640s
ok launchpad.net/juju-core/environs/cloudinit 0.652s
ok launchpad.net/juju-core/environs/config 1.061s
ok launchpad.net/juju-core/environs/dummy 17.147s
ok launchpad.net/juju-core/environs/ec2 175.440s
ok launchpad.net/juju-core/environs/imagemetadata 0.345s
ok launchpad.net/juju-core/environs/instances 0.236s
ok launchpad.net/juju-core/environs/jujutest 0.261s
ok launchpad.net/juju-core/environs/local 1.776s
? launchpad.net/juju-core/environs/local/storage [no test files]
ok launchpad.net/juju-core/environs/localstorage 0.267s
ok laun...

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2013/07/26 01:02:43, axw wrote:
> Please take a look.

Going to go ahead and attempt merge; I just fixed a unit test and it was
trivial.
Someone tell me off if this isn't okay, and I'll wait next time.

https://codereview.appspot.com/11758043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

that's fine. go for it.

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.4 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help into lp:juju-core failed. Below is the output from the failed tests.

agent/agent.go:13:2: import "launchpad.net/goyaml": cannot find package
agent/agent.go:15:2: import "launchpad.net/juju-core/agent/tools": cannot find package
agent/agent.go:16:2: import "launchpad.net/juju-core/errors": cannot find package
agent/agent.go:17:2: import "launchpad.net/juju-core/state": cannot find package
agent/agent.go:18:2: import "launchpad.net/juju-core/state/api": cannot find package
agent/agent.go:19:2: import "launchpad.net/juju-core/state/api/params": cannot find package
agent/agent.go:20:2: import "launchpad.net/juju-core/utils": cannot find package
agent/tools/marshal.go:7:2: import "labix.org/v2/mgo/bson": cannot find package
agent/tools/list.go:9:2: import "launchpad.net/juju-core/utils/set": cannot find package
agent/tools/build.go:17:2: import "launchpad.net/juju-core/version": cannot find package
agent/tools/tools.go:9:2: import "launchpad.net/loggo": cannot find package
charm/meta.go:15:2: import "launchpad.net/juju-core/charm/hooks": cannot find package
charm/dir.go:17:2: import "launchpad.net/juju-core/log": cannot find package
charm/config.go:14:2: import "launchpad.net/juju-core/schema": cannot find package
cmd/cmd.go:16:2: import "launchpad.net/gnuflag": cannot find package
cmd/builddb/main.go:14:2: import "launchpad.net/juju-core/charm": cannot find package
cmd/builddb/main.go:15:2: import "launchpad.net/juju-core/environs": cannot find package
cmd/builddb/main.go:22:2: import "launchpad.net/juju-core/environs/all": cannot find package
cmd/builddb/main.go:16:2: import "launchpad.net/juju-core/juju": cannot find package
cmd/charmd/main.go:15:2: import "launchpad.net/juju-core/store": cannot find package
cmd/charmload/main.go:11:2: import "launchpad.net/lpad": cannot find package
cmd/juju/imagemetadata.go:9:2: import "launchpad.net/goose/identity": cannot find package
cmd/juju/publish.go:9:2: import "launchpad.net/juju-core/bzr": cannot find package
cmd/juju/addmachine.go:9:2: import "launchpad.net/juju-core/cmd": cannot find package
cmd/juju/addmachine.go:10:2: import "launchpad.net/juju-core/constraints": cannot find package
cmd/juju/bootstrap.go:13:2: import "launchpad.net/juju-core/environs/config": cannot find package
cmd/juju/imagemetadata.go:12:2: import "launchpad.net/juju-core/environs/imagemetadata": cannot find package
cmd/juju/bootstrap.go:14:2: import "launchpad.net/juju-core/environs/provider": cannot find package
cmd/juju/synctools.go:11:2: import "launchpad.net/juju-core/environs/sync": cannot find package
cmd/juju/addmachine.go:11:2: import "launchpad.net/juju-core/instance": cannot find package
cmd/juju/addrelation.go:11:2: import "launchpad.net/juju-core/state/statecmd": cannot find package
cmd/juju/helptool.go:13:2: import "launchpad.net/juju-core/worker/uniter/jujuc": cannot find package
cmd/jujud/agent.go:13:2: import "launchpad.net/juju-core/agent": cannot find package
cmd/jujud/upgradevalidation.go:14:2: import "launchpad.net/juju-core/container/lxc": cannot find package
cmd/jujud/upgrade.go:14:2: import "launchpad.net/juju-core/downloader": c...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'cmd/juju/helptool.go'
--- cmd/juju/helptool.go 1970-01-01 00:00:00 +0000
+++ cmd/juju/helptool.go 2013-07-30 07:40:47 +0000
@@ -0,0 +1,104 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package main
5
6import (
7 "fmt"
8
9 "launchpad.net/gnuflag"
10
11 "launchpad.net/juju-core/charm"
12 "launchpad.net/juju-core/cmd"
13 "launchpad.net/juju-core/worker/uniter/jujuc"
14)
15
16// dummyHookContext implements jujuc.Context,
17// as expected by jujuc.NewCommand.
18type dummyHookContext struct{}
19
20func (dummyHookContext) UnitName() string {
21 return ""
22}
23func (dummyHookContext) PublicAddress() (string, bool) {
24 return "", false
25}
26func (dummyHookContext) PrivateAddress() (string, bool) {
27 return "", false
28}
29func (dummyHookContext) OpenPort(protocol string, port int) error {
30 return nil
31}
32func (dummyHookContext) ClosePort(protocol string, port int) error {
33 return nil
34}
35func (dummyHookContext) ConfigSettings() (charm.Settings, error) {
36 return charm.NewConfig().DefaultSettings(), nil
37}
38func (dummyHookContext) HookRelation() (jujuc.ContextRelation, bool) {
39 return nil, false
40}
41func (dummyHookContext) RemoteUnitName() (string, bool) {
42 return "", false
43}
44func (dummyHookContext) Relation(id int) (jujuc.ContextRelation, bool) {
45 return nil, false
46}
47func (dummyHookContext) RelationIds() []int {
48 return []int{}
49}
50
51type HelpToolCommand struct {
52 cmd.CommandBase
53 tool string
54}
55
56func (t *HelpToolCommand) Info() *cmd.Info {
57 return &cmd.Info{
58 Name: "help-tool",
59 Args: "[tool]",
60 Purpose: "show help on a juju charm tool",
61 }
62}
63
64func (t *HelpToolCommand) Init(args []string) error {
65 tool, err := cmd.ZeroOrOneArgs(args)
66 if err == nil {
67 t.tool = tool
68 }
69 return err
70}
71
72func (c *HelpToolCommand) Run(ctx *cmd.Context) error {
73 var hookctx dummyHookContext
74 if c.tool == "" {
75 // Ripped from SuperCommand. We could Run() a SuperCommand
76 // with "help commands", but then the implicit "help" command
77 // shows up.
78 names := jujuc.CommandNames()
79 cmds := make([]cmd.Command, 0, len(names))
80 longest := 0
81 for _, name := range names {
82 if c, err := jujuc.NewCommand(hookctx, name); err == nil {
83 if len(name) > longest {
84 longest = len(name)
85 }
86 cmds = append(cmds, c)
87 }
88 }
89 for _, c := range cmds {
90 info := c.Info()
91 fmt.Fprintf(ctx.Stdout, "%-*s %s\n", longest, info.Name, info.Purpose)
92 }
93 } else {
94 c, err := jujuc.NewCommand(hookctx, c.tool)
95 if err != nil {
96 return err
97 }
98 info := c.Info()
99 f := gnuflag.NewFlagSet(info.Name, gnuflag.ContinueOnError)
100 c.SetFlags(f)
101 ctx.Stdout.Write(info.Help(f))
102 }
103 return nil
104}
0105
=== added file 'cmd/juju/helptool_test.go'
--- cmd/juju/helptool_test.go 1970-01-01 00:00:00 +0000
+++ cmd/juju/helptool_test.go 2013-07-30 07:40:47 +0000
@@ -0,0 +1,63 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package main
5
6import (
7 "strings"
8
9 . "launchpad.net/gocheck"
10 "launchpad.net/juju-core/testing"
11)
12
13type HelpToolSuite struct {
14 home *testing.FakeHome
15}
16
17var _ = Suite(&HelpToolSuite{})
18
19func (suite *HelpToolSuite) SetUpTest(c *C) {
20 suite.home = testing.MakeSampleHome(c)
21}
22
23func (suite *HelpToolSuite) TearDownTest(c *C) {
24 suite.home.Restore()
25}
26
27func (suite *HelpToolSuite) TestHelpToolHelp(c *C) {
28 output := badrun(c, 0, "help", "help-tool")
29 c.Assert(output, Equals, `usage: juju help-tool [tool]
30purpose: show help on a juju charm tool
31`)
32}
33
34func (suite *HelpToolSuite) TestHelpTool(c *C) {
35 expectedNames := []string{
36 "close-port",
37 "config-get",
38 "juju-log",
39 "open-port",
40 "relation-get",
41 "relation-ids",
42 "relation-list",
43 "relation-set",
44 "unit-get",
45 }
46 output := badrun(c, 0, "help-tool")
47 lines := strings.Split(strings.TrimSpace(output), "\n")
48 for i, line := range lines {
49 lines[i] = strings.Fields(line)[0]
50 }
51 c.Assert(lines, DeepEquals, expectedNames)
52}
53
54func (suite *HelpToolSuite) TestHelpToolName(c *C) {
55 output := badrun(c, 0, "help-tool", "relation-get")
56 expectedHelp := `usage: relation-get \[options\] <key> <unit id>
57purpose: get relation settings
58
59options:
60(.|\n)*
61relation-get prints the value(.|\n)*`
62 c.Assert(output, Matches, expectedHelp)
63}
064
=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go 2013-05-28 02:05:06 +0000
+++ cmd/juju/main.go 2013-07-30 07:40:47 +0000
@@ -90,6 +90,9 @@
90 // Charm publishing commands.90 // Charm publishing commands.
91 juju.Register(&PublishCommand{})91 juju.Register(&PublishCommand{})
9292
93 // Charm tool commands.
94 juju.Register(&HelpToolCommand{})
95
93 // Common commands.96 // Common commands.
94 juju.Register(&cmd.VersionCommand{})97 juju.Register(&cmd.VersionCommand{})
9598
9699
=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go 2013-07-10 18:25:42 +0000
+++ cmd/juju/main_test.go 2013-07-30 07:40:47 +0000
@@ -230,6 +230,7 @@
230 "get-env", // alias for get-environment230 "get-env", // alias for get-environment
231 "get-environment",231 "get-environment",
232 "help",232 "help",
233 "help-tool",
233 "image-metadata",234 "image-metadata",
234 "init",235 "init",
235 "publish",236 "publish",

Subscribers

People subscribed via source and target branches

to status/vote changes: