|
|
|
Fix 1299120 command aliases
Fix bug 1299120 by ensuring that the
preferred command is not displayed as
an alias of another command in juju
help commands.
Change file and function names to
be consistant with the preferred
command.
https://code.launchpad.net/~waigani/juju-core/cmd_help_aliases_1299120/+merge/218728
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Fix 1299120 command aliases #
Total comments: 6
Patch Set 3 : Fix 1299120 command aliases #
Total comments: 6
Patch Set 4 : Fix 1299120 command aliases #
Total messages: 9
|
waigani
Please take a look.
|
11 years, 8 months ago (2014年05月08日 02:50:40 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
This breaks compatibility in some places, and we can't do that. The changes in cmd/juju are fine, but anything that changes what we send over the wire for the API is not. https://codereview.appspot.com/92080046/diff/20001/cmd/juju/removemachine.go File cmd/juju/removemachine.go (right): https://codereview.appspot.com/92080046/diff/20001/cmd/juju/removemachine.go#... cmd/juju/removemachine.go:77: return apiclient.ForceReplaceMachines(c.MachineIds...) Replace? https://codereview.appspot.com/92080046/diff/20001/state/api/client.go File state/api/client.go (left): https://codereview.appspot.com/92080046/diff/20001/state/api/client.go#oldcod... state/api/client.go:378: // ServiceDestroy destroys a given service. What about service? Did we keep "destroy" for service but "remove" everything else? that seems somewhat incoherent to me. https://codereview.appspot.com/92080046/diff/20001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/92080046/diff/20001/state/api/client.go#newcod... state/api/client.go:274: func (c *Client) ReplaceMachines(machines ...string) error { This Replace is surprising to me. Just a typo, or something deeper I haven't figured out? https://codereview.appspot.com/92080046/diff/20001/state/api/client.go#newcod... state/api/client.go:375: return c.call("RemoveServiceUnits", params, nil) We definitely can't change the stuff we call() -- we need to keep working with old servers. https://codereview.appspot.com/92080046/diff/20001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/92080046/diff/20001/state/api/params/params.go... state/api/params/params.go:285: type RemoveServiceUnits struct { Yeah, please just don't touch the API at all. We can use the better names with new api versions -- when we get versioning -- but for now this is just a break. (ok, *this* is not -- the names of the types aren't reflected in what we sent over the wire) https://codereview.appspot.com/92080046/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/92080046/diff/20001/state/apiserver/client/cli... state/apiserver/client/client.go:490: func (c *Client) RemoveServiceUnits(args params.RemoveServiceUnits) error { ...but *this* is a break.
Please take a look.
LGTM, but please do the type/var renames for RemoveMachine as well https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go File cmd/juju/main.go (left): https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go#oldcode83 cmd/juju/main.go:83: jujucmd.Register(wrap(&DestroyMachineCommand{})) Shouldn't we RemoveMachine as well? if not, why not? :) https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go File cmd/juju/removemachine.go (left): https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go#... cmd/juju/removemachine.go:24: const destroyMachineDoc = ` removeMachineDoc https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go#... cmd/juju/removemachine.go:31: func (c *DestroyMachineCommand) Info() *cmd.Info { RemoveMachineCommand https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go File cmd/juju/removemachine.go (right): https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go#... cmd/juju/removemachine.go:34: # Remove machine 6, running units or containers Remove machine 6, and all its units and containers?
https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go File cmd/juju/main.go (left): https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go#oldcode83 cmd/juju/main.go:83: jujucmd.Register(wrap(&DestroyMachineCommand{})) On 2014年05月09日 06:51:55, fwereade wrote: > Shouldn't we RemoveMachine as well? if not, why not? :) It was intended to switch to "juju remove-machine" at least in the bug. https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go File cmd/juju/removemachine.go (left): https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go#... cmd/juju/removemachine.go:39: } It seems you can't comment on a rename-only change. I wanted to note that removemachine_test.go certainly seems like it would need updating for the changes of DestroyMachine to RemoveMachine.
On 2014年05月09日 09:24:49, jameinel wrote: > https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go > File cmd/juju/main.go (left): > > https://codereview.appspot.com/92080046/diff/40001/cmd/juju/main.go#oldcode83 > cmd/juju/main.go:83: jujucmd.Register(wrap(&DestroyMachineCommand{})) > On 2014年05月09日 06:51:55, fwereade wrote: > > Shouldn't we RemoveMachine as well? if not, why not? :) > > It was intended to switch to "juju remove-machine" at least in the bug. > > https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go > File cmd/juju/removemachine.go (left): > > https://codereview.appspot.com/92080046/diff/40001/cmd/juju/removemachine.go#... > cmd/juju/removemachine.go:39: } > It seems you can't comment on a rename-only change. I wanted to note that > removemachine_test.go certainly seems like it would need updating for the > changes of DestroyMachine to RemoveMachine. Yes, you're right. It seems I'm being sloppy with my commits. I'd made the changes but managed not to commit them, sorry.