Cadence CLI #577

vancexu · 2018-02-23T02:17:43Z

command line tool for users to perform common tasks with cadence, see readme for details.
Tests will be done in separate PR

coveralls · 2018-02-23T19:58:26Z

Coverage decreased (-3.0004%) to 64.345% when pulling b7f91d8 on cli into 5c6cb08 on master.

wxing1292 · 2018-02-23T21:39:45Z

common/cli/util.go

+		data = e.WorkflowExecutionTerminatedEventAttributes
+
+	default:
+		data = e


this list of events is not complete, at least missing the child workflow events

Good catch, added all

madhuravi · 2018-02-23T21:44:48Z

common/cli/factory.go

+}
+
+// WorkflowClientBuilderInterface is an interface to build client to cadence service
+type WorkflowClientBuilderInterface interface {


Why is this file here? Where is it being used? Can't find any references in the diff.

I previously thought common might be a good place, and later playground or other apps can use it. But now I think it might be better just put everything in cli folder for now, so changed.
This interface is used in "commands.go", and I need it so that I can have a customized builder used internal. See update comments in the file.

yiminc-zz · 2018-02-24T01:10:33Z

tools/cli/app.go

+	app.Flags = []cli.Flag{
+		cli.StringFlag{
+			Name:   FlagAddressWithAlias,
+			Value:  "",


I think we have default value of 127.0.0.1:7933, it is not the case?

Default localHostPort is 127.0.0.1:7933. I put empty string here because it is easier for processing.

yiminc-zz · 2018-02-24T01:15:55Z

tools/cli/app.go

+		cli.BoolFlag{
+			Name:   FlagCustomizeBuilderWithAlias,
+			Usage:  "customize builder function or default",
+			EnvVar: "CADENCE_CUSTOMIZED_BUILDER",


this flag is confusing. How is user suppose to use this flag?

Agree it is confusing, removed.

yiminc-zz · 2018-02-24T01:19:03Z

tools/cli/commands.go

+var colorGreen func(i ...interface{}) string
+
+func init() {
+	colorRed = color.New(color.FgRed).SprintFunc()


could you assign the value when you declare the variables so we don't need this init()?

yiminc-zz · 2018-02-24T01:19:53Z

tools/cli/commands.go

+}
+
+// cBuilder is used to create cadence clients if user want to provide customized builder
+var cBuilder WorkflowClientBuilderInterface


I don't understand why do we need this?

This is necessary for customized builder. Internally, we have special tag like ENV and additional processing when creating builder. By implementing these interfaces and inject with SetBuilder, we can achieve customized builder. This is transparent to CLI user.

yiminc-zz · 2018-02-24T01:23:25Z

tools/cli/commands.go

+}
+
+func newContextForLongPoll() (context.Context, context.CancelFunc) {
+	return context.WithTimeout(context.Background(), time.Minute*2)


Do we need to let user specify timeout for this? (Make it optional, and default to 2 minutes.) For example, what if the workflow takes longer than 2 minutes?

Yes, changed.

mfateev · 2018-02-27T13:49:57Z

tools/cli/README.md

+- You should see an executable `cadence-cli`
+
+## Quick Start
+Run `./cadence-cli` to view help message. There are some top level commands and global options.   


Why is it cadence-cli? The current one is just "cadence".

Renamed to cadence

mfateev · 2018-02-27T13:50:53Z

tools/cli/README.md

+- You should see an executable `cadence-cli`
+
+## Quick Start
+Run `./cadence-cli` to view help message. There are some top level commands and global options.   


I think it should also reply to
cadence help
cadence help domain
cadence help workflow

Let's rename cadence to cadence-server and cadence-cli to cadence then

mfateev · 2018-02-27T13:52:11Z

tools/cli/README.md

+*Note:* make sure you have cadence server running before using CLI 
+
+### Domain operation examples 
+- Register a new domain named "samples-domain":  


Should you reference DOMAIN environment here?
Should we add support for .cadence file that is looked in the parent directories that contains variables like domain?

Good suggestion, I will add DOMAIN env here also.
File support will be done as a feature, together with cache workflow_id and run_id

mfateev · 2018-02-27T13:53:06Z

tools/cli/README.md

+```   
+- View "samples-domain" details:   
+```
+./cadence-cli --domain samples-domain domain describe  


Why we have "show" for workflow and "describe" for domain?

I think "workflow describe" is a little bit confusing which looks like describe detail info of workflow such as workflow type, arguments, wid, runid etc, but workflow show is showing history.

mfateev · 2018-02-27T13:59:57Z

tools/cli/README.md

+./cadence-cli --domain samples-domain workflow start --tl helloWorldGroup --wt main.Workflow --et 60 --dt 10 -i '"vancexu"'
+
+# see more options for workflow start
+./cadence-cli --domain samples-domain workflow start -h


It should be
cadence workflow start help

It think
cadence help

should say to run
cadence workflow help
and
cadence domain help
And cadence workflow help should ask to run
cadence workflow start help

No, cadence workflow start help won't work because there is no help subsubcommands, but only --help or -h command options.

When view help of cadence workflow it will show below:

NAME: cadence workflow - Operate cadence workflow USAGE: cadence workflow command [command options] [arguments...] COMMANDS: show show workflow history start start a new workflow execution run start a new workflow execution and get workflow progress cancel, c cancel a workflow execution signal, s signal a workflow execution terminate, term terminate a new workflow execution list list open or closed workflow executions query query workflow execution OPTIONS: --help, -h show help

the usage will indicate how the help option can be used

mfateev · 2018-02-27T14:02:03Z

tools/cli/README.md

+
+# for workflow with multiple input, seperate each json with space/newline like
+./cadence-cli --domain samples-domain workflow start --tl helloWorldGroup --wt main.WorkflowWith3Args --et 60 --dt 10 -i '"your_input_string" 123 {"Name":"my-string", "Age":12345}'
+```


Can we just make it a single json list?
["your_input_string", 123, {"Name":"my-string", "Age":12345}]
For single value do not require the list wrapper.

Unfortunately we cannot.
Because using your proposal there is no way to tell if ["your_input_string", 123, {"Name":"my-string", "Age":12345}] input is 1 list argument or 3 arguments.

We could say that list always defaults to the list of arguments. If they indeed want to specify list as a single argument (which is very rare) then they have to pass list of lists:
[[...]].

Why is it resolved?

After discussion, we decide to stick with using spaces/newlines separate json, instead of the proposed using list.

mfateev · 2018-02-27T20:04:55Z

tools/cli/README.md

+./cadence-cli --domain samples-domain workflow start --tl helloWorldGroup --wt main.WorkflowWith3Args --et 60 --dt 10 -i '"your_input_string" 123 {"Name":"my-string", "Age":12345}'
+```
+
+- Run workflow and see the progress 


I think this one should come before start as most developers should use it during development. I think you also want to expand on what "run" means in this context and what benefits over start it has.

mfateev · 2018-02-27T20:05:54Z

tools/cli/README.md

+# then you can just
+./cadence-cli domain desc
+./cadence-cli workflow list
+./cadence-cli workflow show -w greetings_38a9e481-a00c-433a-b1ff-b6f67536efd8


Can we make -w optional after show? Just workflow show UUID?

With the cli framework we currently use, it would be a little bit tricky to achieve this requirement. Because show commands also accepts other optional arguments.
Instead, I propose to add new command like showid <wid> as a separate command which takes only wid and have no other arguments to achieve this. what do you think?

If it is indeed that hard then showid is OK.

Cool, added showid as a shortcut of show -w <wid> -r <rid>

yiminc-zz · 2018-02-27T21:59:21Z

tools/cli/factory.go

+// Usually default WorkflowClientBuilder is enough for development purpose, but
+// user can still customized builder by implementing this interface, and call SetBulder after initialize cli.
+// The customized builder may have more processing on Env, Address and other info.
+type WorkflowClientBuilderInterface interface {


I thought we talked about this interface, and decided that it only need one method:
CreateServiceClient(ctx *cli.Context). Or am I missing something?

Changed, I missed one part previously. Thanks

mfateev · 2018-02-28T22:54:38Z

tools/cli/README.md

+
+- Run workflow: Start a workflow and see it's progress 
+```
+./cadence workflow run --tl helloWorldGroup --wt main.Workflow --et 60 --dt 10 -i '"cadence"'


Let's default decision task timeout to something like 5 seconds and do not require it.

Done, default 10 sec

mfateev · 2018-02-28T22:55:32Z

tools/cli/README.md

+### Workflow operation examples
+(The following examples assume you already export CADENCE_CLI_DOMAIN environment variable as Tips above)
+
+- Run workflow: Start a workflow and see it's progress 


Add that this command doesn't finish until workflow completes.

mfateev · 2018-02-28T22:56:43Z

tools/cli/README.md

+- List open or closed workflow executions
+```
+./cadence workflow list
+```


What about query?

Added. Need to add samples to make it looks better later

mfateev · 2018-02-28T22:59:40Z

tools/cli/README.md

+
+# for workflow with multiple input, seperate each json with space/newline like
+./cadence-cli --domain samples-domain workflow start --tl helloWorldGroup --wt main.WorkflowWith3Args --et 60 --dt 10 -i '"your_input_string" 123 {"Name":"my-string", "Age":12345}'
+```


We could say that list always defaults to the list of arguments. If they indeed want to specify list as a single argument (which is very rare) then they have to pass list of lists:
[[...]].

yiminc-zz · 2018-02-28T22:47:17Z

cmd/tools/cli/main.go

+)
+
+// Start using this CLI tool with command
+// `./cadence-cli` to show help message


update the comments to remove -cli.

yiminc-zz · 2018-02-28T22:52:05Z

tools/cli/factory.go

+type WorkflowClientBuilder struct {
+	hostPort   string
+	dispatcher *yarpc.Dispatcher
+	Logger     *zap.Logger


why is this field public? Also, does the WorkflowClientBuilder needs to be public?

Good catch, changed Logger to logger.
WorkflowClientBuilder is needed, otherwise lint complain on NewBuilder function.

yiminc-zz · 2018-02-28T23:45:22Z

cmd/tools/cli/main.go

+func main() {
+	app := cli.NewCliApp()
+
+	logger, err := zap.NewDevelopment()


Can we not make this logger a required argument to NewBuilder()? It could create a default inside that builder. Just want to keep code here to be minimal.

yiminc-zz · 2018-02-28T23:47:09Z

cmd/tools/cli/main.go

+	if err != nil {
+		panic(err)
+	}
+	cli.SetBuilder(cli.NewBuilder(logger))


I would even make this call optional and use cli.NewBuilder() if it is not set.
Actually, I would check the builder inside the cli.NewCliApp() and set it to the default value if it is not set yet. So in internal version, you can call the cli.SetBuilder() before cli.NewCliApp().

Make sense I like it. Updated.

mfateev · 2018-03-01T06:22:41Z

tools/cli/README.md

+- Query workflow execution
+```
+# use build-in query type "__stack_trace" which is supported by cadence client library
+./cadence workflow query -w <wid> -r <rid> 


What about
cadence workflow stack -w ...

I think query at least link me with QueryWorkflow of cadence-client, no sure why you propose stack?

Because if you don't specify query type you default to stack which is confusing. I would require query type, but provide separate command "stack" which would execute query type "__stack_trace" which is hard to remember.

Added stack command

mfateev · 2018-03-01T06:24:04Z

tools/cli/README.md

+
+- Start workflow: 
+```
+./cadence workflow start --tl helloWorldGroup --wt main.Workflow --et 60 --dt 10 -i '"cadence"'


I would change ./cadence to cadence in all these samples. In most cases it would be installed though brew or be somewhere in the PATH.

For this readme, I think ./cadence is better because reader can easily understand it is the executable after running make, and "advanced player" can obviously use PATH and replace it to whatever they what.
Directly put cadence instead of ./cadence will confuse beginner, if I don't have instructions on how to set PATH or use brew. And for the simplicity of this readme, I don't want to include those.

mfateev · 2018-03-01T06:25:20Z

tools/cli/README.md

+./cadence workflow query -w <wid> -r <rid> --qt <query-type>
+```
+
+- Signal, cancel, terminate workflow


We have the open activities visibility query. Where is it?
Also we have list of workers connected to a task list. Where is it?

Create #588 and will work on separate pull request.

yiminc-zz

code lgtm.

vancexu requested review from samarabbas and yiminc-zz February 23, 2018 02:17

wxing1292 reviewed Feb 23, 2018

View reviewed changes

madhuravi reviewed Feb 23, 2018

View reviewed changes

yiminc-zz reviewed Feb 24, 2018

View reviewed changes

vancexu mentioned this pull request Feb 26, 2018

Cadence CLI #215

Closed

mfateev reviewed Feb 27, 2018

View reviewed changes

yiminc-zz reviewed Feb 27, 2018

View reviewed changes

mfateev reviewed Feb 28, 2018

View reviewed changes

yiminc-zz reviewed Feb 28, 2018

View reviewed changes

mfateev reviewed Mar 1, 2018

View reviewed changes

yiminc-zz approved these changes Mar 1, 2018

View reviewed changes

mfateev approved these changes Mar 2, 2018

View reviewed changes

Cadence CLI

b7f91d8

vancexu force-pushed the cli branch from 14b372a to b7f91d8 Compare March 2, 2018 19:01

vancexu merged commit 4f0eb40 into master Mar 2, 2018

vancexu deleted the cli branch March 2, 2018 19:50

Cadence CLI #577

Cadence CLI #577

Conversation

vancexu commented Feb 23, 2018 • edited Loading

coveralls commented Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiminc-zz left a comment

Choose a reason for hiding this comment

vancexu commented Feb 23, 2018 •

edited

Loading

coveralls commented Feb 23, 2018 •

edited

Loading

vancexu Feb 27, 2018 •

edited

Loading

vancexu Feb 27, 2018 •

edited

Loading

vancexu Feb 27, 2018 •

edited

Loading

vancexu Mar 1, 2018 •

edited

Loading