-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Copy run-action command to run with feature flag #10568
Conversation
b9570ce
to
9f6f40e
Compare
91a3eaa
to
32de276
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good \o/
Please address minor comments :D
cmd/juju/action/call.go
Outdated
return modelcmd.Wrap(&callCommand{}) | ||
} | ||
|
||
// runCommand enqueues an Action for running on the given unit with given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callCommand...
juju call mysql/3 backup --wait | ||
juju call mysql/3 backup | ||
juju call mysql/leader backup | ||
juju show-action-output <ID> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that you need 'show-action-output example among 'call' command examples :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show-action-output is still a valid command to see the result of calling an action.
It will be renamed however to show-operation in the next PR
cmd/juju/action/call.go
Outdated
for _, arg := range args[len(c.unitReceivers)+1:] { | ||
thisArg := strings.SplitN(arg, "=", 2) | ||
if len(thisArg) != 2 { | ||
return errors.Errorf("argument %q must be of the form key...=value", arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe best to say key.key.key...=value
as per Info().Args?
cmd/juju/action/call.go
Outdated
"and contain only lowercase alphanumeric and hyphens", key) | ||
} | ||
} | ||
// c.args={..., [key, key, key, key, value]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove me? or re-phrase me :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing code, removed in next PR but will do so here instead
return err | ||
} | ||
|
||
// Legacy Juju 1.25 output format for a single unit, no wait. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG, are we still supporting this? Originally we said 'only for the life of trusty'... well, it's past EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in next PR
} | ||
|
||
func (c *callCommand) Run(ctx *cmd.Context) error { | ||
if err := c.ensureAPI(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of a problem since we close api later on but this method can still return it. I do prefer commands having function to obtain new API rather than caching the api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is all existing, just a straight copy. Will try to clean up as work progresses
cmd/juju/action/call.go
Outdated
} | ||
} | ||
|
||
out := make(map[string]interface{}, len(results.Results)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is so close to c.out... Can it be named something else, say 'results'?
@@ -645,6 +645,7 @@ func (s *MainSuite) TestHelpCommands(c *gc.C) { | |||
cmdSet := set.NewStrings(commandNames...) | |||
if !featureflag.Enabled(feature.JujuV3) { | |||
cmdSet.Add("run-action") | |||
cmdSet.Add("run") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"call"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"run" is what's needed here.
If feature flag is not enabled, there is a run command
5c085a1
to
9974123
Compare
|
9974123
to
c742cbb
Compare
|
Description of change
To prepare for implementing the new juju call (action), copy the existing juju run-action command so that the new version can be easily changed, leaving the legacy run-action version untouched. The changes will be significant and easier to do separately in a new command rather than adding feature flag logic to the existing run-action command.
The runaction(_test).go and call(_test).go files are straight copies of run(_test).go
QA steps
ensure juju run-action works still