juju-core

Merge lp:~axwalk/juju-core/lp1182898-add-version-flag into lp:~go-bot/juju-core/trunk

  1. lp1182898-add-version-flag
  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: 1558
Proposed branch: lp:~axwalk/juju-core/lp1182898-add-version-flag
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 180 lines (+97/-10)
2 files modified
cmd/supercommand.go (+36/-10)
cmd/supercommand_test.go (+61/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1182898-add-version-flag
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+176849@code.launchpad.net

Commit message

cmd: Add --version to cmd.SuperCommand

There's no option to control the format;
the sub-command must be used for that.

https://codereview.appspot.com/11808044/

Description of the change

cmd: Add --version to cmd.SuperCommand

There's no option to control the format;
the sub-command must be used for that.

https://codereview.appspot.com/11808044/

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

Reviewers: mp+176849_code.launchpad.net,

Message:
Please take a look.

Description:
cmd: Add --version to cmd.SuperCommand

There's no option to control the format;
the sub-command must be used for that.

https://code.launchpad.net/~axwalk/juju-core/lp1182898-add-version-flag/+merge/176849

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/supercommand.go
   M cmd/supercommand_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130724165437-wt01mn3gqkwfrel0
+New revision: <email address hidden>

Index: cmd/supercommand.go
=== modified file 'cmd/supercommand.go'
--- cmd/supercommand.go 2013-07-10 18:25:42 +0000
+++ cmd/supercommand.go 2013-07-25 03:47:47 +0000
@@ -6,6 +6,7 @@
  import (
   "bytes"
   "fmt"
+ "io/ioutil"
   "sort"
   "strings"

@@ -65,9 +66,11 @@
   Doc string
   Log *Log
   subcmds map[string]Command
+ commonflags *gnuflag.FlagSet
   flags *gnuflag.FlagSet
   subcmd Command
   showHelp bool
+ showVersion bool
   missingCallback MissingCallback
  }

@@ -172,15 +175,25 @@

  const helpPurpose = "show help on a command or other topic"

-// SetFlags adds the options that apply to all commands, particularly those
-// due to logging.
-func (c *SuperCommand) SetFlags(f *gnuflag.FlagSet) {
+// setCommonFlags adds the options that apply to all commands,
+// particularly those due to logging.
+func (c *SuperCommand) setCommonFlags(f *gnuflag.FlagSet) {
   if c.Log != nil {
    c.Log.AddFlags(f)
   }
   f.BoolVar(&c.showHelp, "h", false, helpPurpose)
   f.BoolVar(&c.showHelp, "help", false, "")

+ c.commonflags = gnuflag.NewFlagSet(c.Info().Name, gnuflag.ContinueOnError)
+ c.commonflags.SetOutput(ioutil.Discard)
+ f.VisitAll(func(flag *gnuflag.Flag) {
+ c.commonflags.Var(flag.Value, flag.Name, flag.Usage)
+ })
+}
+
+func (c *SuperCommand) SetFlags(f *gnuflag.FlagSet) {
+ c.setCommonFlags(f)
+ f.BoolVar(&c.showVersion, "version", false, "Show the version of juju")
   c.flags = f
  }

@@ -207,11 +220,11 @@
    return fmt.Errorf("unrecognized command: %s %s", c.Name, args[0])
   }
   args = args[1:]
- c.subcmd.SetFlags(c.flags)
- if err := c.flags.Parse(true, args); err != nil {
+ c.subcmd.SetFlags(c.commonflags)
+ if err := c.commonflags.Parse(true, args); err != nil {
    return err
   }
- args = c.flags.Args()
+ args = c.commonflags.Args()
   if c.showHelp {
    // We want to treat help for the command the same way we would if we
went "help foo".
    args = []string{c.subcmd.Info().Name}
@@ -307,7 +320,7 @@
  `)

   f := gnuflag.NewFlagSet("", gnuflag.ContinueOnError)
- c.super.SetFlags(f)
+ c.super.setCommonFlags(f)
   f.SetOutput(buf)
   f.PrintDefaults()
   return buf.String()
@@ -355,6 +368,13 @@
  }

  func (c *helpCommand) Run(ctx *Context) error {
+ if c.super.showVersion {
+ var v VersionCommand
+ v.SetFlags(c.super.flags)
+ v.Init([]string{})
+ return v...

Read more...

Revision history for this message
William Reade (fwereade) wrote :

There's at least one command (sync-tools?) that itself accepts
--version, with a different meaning. It's not immediately obvious what
happens when the two collide; this needs either to handle that case or
to briefly note how it does so.

https://codereview.appspot.com/11808044/

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

Please take a look.

https://codereview.appspot.com/11808044/

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

On 2013/07/25 09:36:27, fwereade wrote:
> There's at least one command (sync-tools?) that itself accepts
 --version, with a
> different meaning. It's not immediately obvious what happens when the
 two
> collide; this needs either to handle that case or to briefly note how
 it does
> so.

upgrade-juju has --version, and yes this deals with that.
I've added a comment; hope that is clearer.

P.S. Is there a way to re-propose without auto-adding the "PTAL"
message?

https://codereview.appspot.com/11808044/

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

LGTM, nice approach.
it would be nice if the fact that you can pass a --version
flag to juju was documented in the help somewhere.
I can't think of a nice place to put it though,
so I'm happy with it living as an easter egg
for backward compatibility.

https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go#newcode194
cmd/supercommand.go:194: func (c *SuperCommand) SetFlags(f
*gnuflag.FlagSet) {
i think we should keep the doc comment the same here.

https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go#newcode377
cmd/supercommand.go:377: v.Init([]string{})
s/[]string{}/nil/

https://codereview.appspot.com/11808044/

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

On 25 July 2013 11:10, Andrew Wilkins <email address hidden> wrote:
> On 2013/07/25 09:36:27, fwereade wrote:
> P.S. Is there a way to re-propose without auto-adding the "PTAL"
> message?

unfortunately not. those messages are pretty much noise, which is a pity.

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

Please take a look.

https://codereview.appspot.com/11808044/

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

On 2013/07/25 10:23:19, rog wrote:
> LGTM, nice approach.
> it would be nice if the fact that you can pass a --version
> flag to juju was documented in the help somewhere.
> I can't think of a nice place to put it though,
> so I'm happy with it living as an easter egg
> for backward compatibility.

> https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go
> File cmd/supercommand.go (right):

https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go#newcode194
> cmd/supercommand.go:194: func (c *SuperCommand) SetFlags(f
 *gnuflag.FlagSet) {
> i think we should keep the doc comment the same here.

Done. I shuffled the other comments around a bit too.

https://codereview.appspot.com/11808044/diff/5001/cmd/supercommand.go#newcode377
> cmd/supercommand.go:377: v.Init([]string{})
> s/[]string{}/nil/

Done.

https://codereview.appspot.com/11808044/

Revision history for this message
William Reade (fwereade) wrote :

Thanks, that's much clearer. LGTM with a test that explicitly exercises
the no-collision.

https://codereview.appspot.com/11808044/diff/11001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/11808044/diff/11001/cmd/supercommand_test.go#newcode134
cmd/supercommand_test.go:134: }
How about an additional test checking explicitly that --version does not
collide?

https://codereview.appspot.com/11808044/

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

Please take a look.

https://codereview.appspot.com/11808044/

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

The attempt to merge lp:~axwalk/juju-core/lp1182898-add-version-flag 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/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": cannot find package
cmd/jujud/bootstrap.go:15:2: import "launchpad.net/juju-core/environs/cloudinit": cann...

Read more...

Revision history for this message
Go Bot (go-bot) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/supercommand.go'
2--- cmd/supercommand.go 2013-07-10 18:25:42 +0000
3+++ cmd/supercommand.go 2013-07-30 07:31:33 +0000
4@@ -6,6 +6,7 @@
5 import (
6 "bytes"
7 "fmt"
8+ "io/ioutil"
9 "sort"
10 "strings"
11
12@@ -65,9 +66,11 @@
13 Doc string
14 Log *Log
15 subcmds map[string]Command
16+ commonflags *gnuflag.FlagSet
17 flags *gnuflag.FlagSet
18 subcmd Command
19 showHelp bool
20+ showVersion bool
21 missingCallback MissingCallback
22 }
23
24@@ -172,15 +175,31 @@
25
26 const helpPurpose = "show help on a command or other topic"
27
28+// setCommonFlags creates a new "commonflags" flagset, whose
29+// flags are shared with the argument f; this enables us to
30+// add non-global flags to f, which do not carry into subcommands.
31+func (c *SuperCommand) setCommonFlags(f *gnuflag.FlagSet) {
32+ if c.Log != nil {
33+ c.Log.AddFlags(f)
34+ }
35+ f.BoolVar(&c.showHelp, "h", false, helpPurpose)
36+ f.BoolVar(&c.showHelp, "help", false, "")
37+
38+ c.commonflags = gnuflag.NewFlagSet(c.Info().Name, gnuflag.ContinueOnError)
39+ c.commonflags.SetOutput(ioutil.Discard)
40+ f.VisitAll(func(flag *gnuflag.Flag) {
41+ c.commonflags.Var(flag.Value, flag.Name, flag.Usage)
42+ })
43+}
44+
45 // SetFlags adds the options that apply to all commands, particularly those
46 // due to logging.
47 func (c *SuperCommand) SetFlags(f *gnuflag.FlagSet) {
48- if c.Log != nil {
49- c.Log.AddFlags(f)
50- }
51- f.BoolVar(&c.showHelp, "h", false, helpPurpose)
52- f.BoolVar(&c.showHelp, "help", false, "")
53-
54+ c.setCommonFlags(f)
55+ // Only flags set by setCommonFlags are passed on to subcommands.
56+ // Any flags added below only take effect when no subcommand is
57+ // specified (e.g. juju --version).
58+ f.BoolVar(&c.showVersion, "version", false, "Show the version of juju")
59 c.flags = f
60 }
61
62@@ -207,11 +226,11 @@
63 return fmt.Errorf("unrecognized command: %s %s", c.Name, args[0])
64 }
65 args = args[1:]
66- c.subcmd.SetFlags(c.flags)
67- if err := c.flags.Parse(true, args); err != nil {
68+ c.subcmd.SetFlags(c.commonflags)
69+ if err := c.commonflags.Parse(true, args); err != nil {
70 return err
71 }
72- args = c.flags.Args()
73+ args = c.commonflags.Args()
74 if c.showHelp {
75 // We want to treat help for the command the same way we would if we went "help foo".
76 args = []string{c.subcmd.Info().Name}
77@@ -307,7 +326,7 @@
78 `)
79
80 f := gnuflag.NewFlagSet("", gnuflag.ContinueOnError)
81- c.super.SetFlags(f)
82+ c.super.setCommonFlags(f)
83 f.SetOutput(buf)
84 f.PrintDefaults()
85 return buf.String()
86@@ -355,6 +374,13 @@
87 }
88
89 func (c *helpCommand) Run(ctx *Context) error {
90+ if c.super.showVersion {
91+ var v VersionCommand
92+ v.SetFlags(c.super.flags)
93+ v.Init(nil)
94+ return v.Run(ctx)
95+ }
96+
97 // If there is no help topic specified, print basic usage.
98 if c.topic == "" {
99 if _, ok := c.topics["basics"]; ok {
100
101=== modified file 'cmd/supercommand_test.go'
102--- cmd/supercommand_test.go 2013-07-09 10:32:23 +0000
103+++ cmd/supercommand_test.go 2013-07-30 07:31:33 +0000
104@@ -4,8 +4,10 @@
105 package cmd_test
106
107 import (
108+ "bytes"
109 "fmt"
110
111+ "launchpad.net/gnuflag"
112 . "launchpad.net/gocheck"
113
114 "launchpad.net/juju-core/cmd"
115@@ -103,6 +105,65 @@
116 c.Assert(info.Doc, Matches, commandsDoc+helpText)
117 }
118
119+type testVersionFlagCommand struct {
120+ cmd.CommandBase
121+ version string
122+}
123+
124+func (c *testVersionFlagCommand) Info() *cmd.Info {
125+ return &cmd.Info{Name: "test"}
126+}
127+
128+func (c *testVersionFlagCommand) SetFlags(f *gnuflag.FlagSet) {
129+ f.StringVar(&c.version, "version", "", "")
130+}
131+
132+func (c *testVersionFlagCommand) Run(_ *cmd.Context) error {
133+ return nil
134+}
135+
136+func (s *SuperCommandSuite) TestVersionFlag(c *C) {
137+ jc := cmd.NewSuperCommand(cmd.SuperCommandParams{
138+ Name: "jujutest",
139+ Purpose: "to be purposeful",
140+ Doc: "doc\nblah\ndoc",
141+ })
142+ testVersionFlagCommand := &testVersionFlagCommand{}
143+ jc.Register(&cmd.VersionCommand{})
144+ jc.Register(testVersionFlagCommand)
145+
146+ var stdout, stderr bytes.Buffer
147+ ctx := &cmd.Context{
148+ Stdout: &stdout,
149+ Stderr: &stderr,
150+ }
151+
152+ // baseline: juju version
153+ code := cmd.Main(jc, ctx, []string{"version"})
154+ c.Check(code, Equals, 0)
155+ baselineStderr := stderr.String()
156+ baselineStdout := stdout.String()
157+ stderr.Reset()
158+ stdout.Reset()
159+
160+ // juju --version output should match that of juju version.
161+ code = cmd.Main(jc, ctx, []string{"--version"})
162+ c.Check(code, Equals, 0)
163+ c.Assert(stderr.String(), Equals, baselineStderr)
164+ c.Assert(stdout.String(), Equals, baselineStdout)
165+ stderr.Reset()
166+ stdout.Reset()
167+
168+ // juju test --version should update testVersionFlagCommand.version,
169+ // and there should be no output. The --version flag on the 'test'
170+ // subcommand has a different type to the "juju --version" flag.
171+ code = cmd.Main(jc, ctx, []string{"test", "--version=abc.123"})
172+ c.Check(code, Equals, 0)
173+ c.Assert(stderr.String(), Equals, "")
174+ c.Assert(stdout.String(), Equals, "")
175+ c.Assert(testVersionFlagCommand.version, Equals, "abc.123")
176+}
177+
178 func (s *SuperCommandSuite) TestLogging(c *C) {
179 jc := cmd.NewSuperCommand(cmd.SuperCommandParams{Name: "jujutest", Log: &cmd.Log{}})
180 jc.Register(&TestCommand{Name: "blah"})

Subscribers

People subscribed via source and target branches

to status/vote changes: