juju-core

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

  1. lp1186969-add-jujud-tools-help
  2. Merge into 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 :

Please take a look.

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

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 :

LGTM

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

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 :

Please take a look.

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

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
1=== added file 'cmd/juju/helptool.go'
2--- cmd/juju/helptool.go 1970-01-01 00:00:00 +0000
3+++ cmd/juju/helptool.go 2013-07-30 07:40:47 +0000
4@@ -0,0 +1,104 @@
5+// Copyright 2013 Canonical Ltd.
6+// Licensed under the AGPLv3, see LICENCE file for details.
7+
8+package main
9+
10+import (
11+ "fmt"
12+
13+ "launchpad.net/gnuflag"
14+
15+ "launchpad.net/juju-core/charm"
16+ "launchpad.net/juju-core/cmd"
17+ "launchpad.net/juju-core/worker/uniter/jujuc"
18+)
19+
20+// dummyHookContext implements jujuc.Context,
21+// as expected by jujuc.NewCommand.
22+type dummyHookContext struct{}
23+
24+func (dummyHookContext) UnitName() string {
25+ return ""
26+}
27+func (dummyHookContext) PublicAddress() (string, bool) {
28+ return "", false
29+}
30+func (dummyHookContext) PrivateAddress() (string, bool) {
31+ return "", false
32+}
33+func (dummyHookContext) OpenPort(protocol string, port int) error {
34+ return nil
35+}
36+func (dummyHookContext) ClosePort(protocol string, port int) error {
37+ return nil
38+}
39+func (dummyHookContext) ConfigSettings() (charm.Settings, error) {
40+ return charm.NewConfig().DefaultSettings(), nil
41+}
42+func (dummyHookContext) HookRelation() (jujuc.ContextRelation, bool) {
43+ return nil, false
44+}
45+func (dummyHookContext) RemoteUnitName() (string, bool) {
46+ return "", false
47+}
48+func (dummyHookContext) Relation(id int) (jujuc.ContextRelation, bool) {
49+ return nil, false
50+}
51+func (dummyHookContext) RelationIds() []int {
52+ return []int{}
53+}
54+
55+type HelpToolCommand struct {
56+ cmd.CommandBase
57+ tool string
58+}
59+
60+func (t *HelpToolCommand) Info() *cmd.Info {
61+ return &cmd.Info{
62+ Name: "help-tool",
63+ Args: "[tool]",
64+ Purpose: "show help on a juju charm tool",
65+ }
66+}
67+
68+func (t *HelpToolCommand) Init(args []string) error {
69+ tool, err := cmd.ZeroOrOneArgs(args)
70+ if err == nil {
71+ t.tool = tool
72+ }
73+ return err
74+}
75+
76+func (c *HelpToolCommand) Run(ctx *cmd.Context) error {
77+ var hookctx dummyHookContext
78+ if c.tool == "" {
79+ // Ripped from SuperCommand. We could Run() a SuperCommand
80+ // with "help commands", but then the implicit "help" command
81+ // shows up.
82+ names := jujuc.CommandNames()
83+ cmds := make([]cmd.Command, 0, len(names))
84+ longest := 0
85+ for _, name := range names {
86+ if c, err := jujuc.NewCommand(hookctx, name); err == nil {
87+ if len(name) > longest {
88+ longest = len(name)
89+ }
90+ cmds = append(cmds, c)
91+ }
92+ }
93+ for _, c := range cmds {
94+ info := c.Info()
95+ fmt.Fprintf(ctx.Stdout, "%-*s %s\n", longest, info.Name, info.Purpose)
96+ }
97+ } else {
98+ c, err := jujuc.NewCommand(hookctx, c.tool)
99+ if err != nil {
100+ return err
101+ }
102+ info := c.Info()
103+ f := gnuflag.NewFlagSet(info.Name, gnuflag.ContinueOnError)
104+ c.SetFlags(f)
105+ ctx.Stdout.Write(info.Help(f))
106+ }
107+ return nil
108+}
109
110=== added file 'cmd/juju/helptool_test.go'
111--- cmd/juju/helptool_test.go 1970-01-01 00:00:00 +0000
112+++ cmd/juju/helptool_test.go 2013-07-30 07:40:47 +0000
113@@ -0,0 +1,63 @@
114+// Copyright 2013 Canonical Ltd.
115+// Licensed under the AGPLv3, see LICENCE file for details.
116+
117+package main
118+
119+import (
120+ "strings"
121+
122+ . "launchpad.net/gocheck"
123+ "launchpad.net/juju-core/testing"
124+)
125+
126+type HelpToolSuite struct {
127+ home *testing.FakeHome
128+}
129+
130+var _ = Suite(&HelpToolSuite{})
131+
132+func (suite *HelpToolSuite) SetUpTest(c *C) {
133+ suite.home = testing.MakeSampleHome(c)
134+}
135+
136+func (suite *HelpToolSuite) TearDownTest(c *C) {
137+ suite.home.Restore()
138+}
139+
140+func (suite *HelpToolSuite) TestHelpToolHelp(c *C) {
141+ output := badrun(c, 0, "help", "help-tool")
142+ c.Assert(output, Equals, `usage: juju help-tool [tool]
143+purpose: show help on a juju charm tool
144+`)
145+}
146+
147+func (suite *HelpToolSuite) TestHelpTool(c *C) {
148+ expectedNames := []string{
149+ "close-port",
150+ "config-get",
151+ "juju-log",
152+ "open-port",
153+ "relation-get",
154+ "relation-ids",
155+ "relation-list",
156+ "relation-set",
157+ "unit-get",
158+ }
159+ output := badrun(c, 0, "help-tool")
160+ lines := strings.Split(strings.TrimSpace(output), "\n")
161+ for i, line := range lines {
162+ lines[i] = strings.Fields(line)[0]
163+ }
164+ c.Assert(lines, DeepEquals, expectedNames)
165+}
166+
167+func (suite *HelpToolSuite) TestHelpToolName(c *C) {
168+ output := badrun(c, 0, "help-tool", "relation-get")
169+ expectedHelp := `usage: relation-get \[options\] <key> <unit id>
170+purpose: get relation settings
171+
172+options:
173+(.|\n)*
174+relation-get prints the value(.|\n)*`
175+ c.Assert(output, Matches, expectedHelp)
176+}
177
178=== modified file 'cmd/juju/main.go'
179--- cmd/juju/main.go 2013-05-28 02:05:06 +0000
180+++ cmd/juju/main.go 2013-07-30 07:40:47 +0000
181@@ -90,6 +90,9 @@
182 // Charm publishing commands.
183 juju.Register(&PublishCommand{})
184
185+ // Charm tool commands.
186+ juju.Register(&HelpToolCommand{})
187+
188 // Common commands.
189 juju.Register(&cmd.VersionCommand{})
190
191
192=== modified file 'cmd/juju/main_test.go'
193--- cmd/juju/main_test.go 2013-07-10 18:25:42 +0000
194+++ cmd/juju/main_test.go 2013-07-30 07:40:47 +0000
195@@ -230,6 +230,7 @@
196 "get-env", // alias for get-environment
197 "get-environment",
198 "help",
199+ "help-tool",
200 "image-metadata",
201 "init",
202 "publish",

Subscribers

People subscribed via source and target branches

to status/vote changes: