Skip to content
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

Support multiple input options in cli #600

Merged
merged 4 commits into from
Jul 24, 2020
Merged

Support multiple input options in cli #600

merged 4 commits into from
Jul 24, 2020

Conversation

feedmeapples
Copy link
Contributor

@feedmeapples feedmeapples commented Jul 23, 2020

What changed?
Fixes #502
Added support for multiple input payloads
tctl workflow start --tq helloWorldGroup --wt main.WorkflowWithInputs --et 60 -i '{"Joker":"Heheheh", "Harley":"Pumpkin!"}' -i '"to infinity and beyond"' -i 'null'

Why?
Before you could specify only one payload as --input

How did you test it?
Ran the command. Ran tests

Potential risks
Low risk. The code shares some logic with --memo flag

@feedmeapples feedmeapples marked this pull request as draft July 23, 2020 22:17
@feedmeapples feedmeapples marked this pull request as ready for review July 24, 2020 01:42
Name: FlagInputWithAlias,
Usage: "Optional input for the workflow, in JSON format. If there are multiple parameters, concatenate them and separate by space.",
Usage: "Optional input for the workflow, in JSON format. If there are multiple parameters, pass each as a separate input flag.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit] do we need to mention anything about the input flag ordering corresponding to the input on the workflow params, or is this implicit enough for a user to get?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good point. I would add something like "Order of input flags must be the same as parameters order."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think they will expect the ordering to matter, i would rather keep the help page relatively simple, focusing on the main information. However we could add this to our docs. I've added an item to Cully regarding the docs


var jsons []interface{}
for _, jsonRaw := range jsonsRaw {
if len(jsonRaw) == 0 {
Copy link
Member

@mastermanu mastermanu Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the scenario for this? passing an intentionally empty argument? Why would we not just ErrorAndExit in this case? Also, if I have multiple empty arguments and we don't append it to the json array, how do I know which empty argument maps with which order of the parameter?

Copy link
Member

@alexshtin alexshtin Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we pass nil? --input "null" should work, right? If yes, I would validate this case and ErrorAndExit("Empty input flag. Pass \"null\" for nil parameter value.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using continue was indeed not good.
Added support for passing null

inputsRaw = append(inputsRaw, []byte(i))
}
return inputsRaw
} else if c.IsSet(flagInputFileName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is multiple-file input in the next PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope so.

Copy link
Member

@mastermanu mastermanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments. nothing major though

var inputsRaw [][]byte
for _, i := range *inputs {
if i == "nil" {
ErrorAndExit("Empty input flag. Pass \"null\" for nil parameter value.", nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add this to the help description up above? Folks that don't use GoLang won't even try to use "nil" and so this error message is going to be hard to discover. Then I would just remove this.

for _, i := range *inputs {
if i == "nil" {
ErrorAndExit("Empty input flag. Pass \"null\" for nil parameter value.", nil)
} else if i == "null" {
Copy link
Member

@mastermanu mastermanu Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit] use string comparison that is case insensitive (https://golang.org/pkg/strings/#example_EqualFold)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI workflow run command can't process --input param with multiple args
3 participants