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

Add support for template arguments #179

Merged
merged 6 commits into from Apr 7, 2017
Merged

Add support for template arguments #179

merged 6 commits into from Apr 7, 2017

Conversation

prashantv
Copy link
Contributor

Template arguments are of the format ${argname:fallback}. Arguments can
be specified on the commandline using --arg or -A. Arguments are
parsed as YAML values, and allow more complex types (e.g., lists) to be
specified as arguments.

We only use template arguments for request bodies currently.

Argument parsing is based on @abhinav's work for YARPC configuration.

@mention-bot
Copy link

@prashantv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kriskowal, @blampe and @eklitzke to be potential reviewers.

Value string
}

// Parse parses strings in the format, ${NAME} and ${NAME:VALUE}.
Copy link
Contributor

@kriskowal kriskowal Mar 14, 2017

Choose a reason for hiding this comment

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

Add a Lit struct and return Term ( Lit | Var). Lit should produce a string with \$ => $ and \\ => \.

return processMap(req, argsUnmarshalled)
}

func processString(v string, args map[string]interface{}) (interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should unconditionally parse the string in its DSL, including escaping \$ and \\. The grammar should return a simple AST with either a Lit or a Var term, and this function should just switch on that.

This leaves open the possibility of extending the grammar to support string interpolation like ${host}:${port}. We can forbid this for now.

@@ -0,0 +1,3 @@
service: foo
request:
name: ${user:foo
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs test for \${user}. Would be good to test \\, \$.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to test requests with embedded non-string arguments, e.g., as in yab -A peer-list:$(cat peers.yaml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cover a lot of the edge cases such as \\ and \$ in TestProcessString and TestProcessMap already. but added a couple more to args.yaml -- it has a fallback which is not a string, a list replacement, and testing for escapes.

@prashantv prashantv changed the base branch from peer_list_opts to dev March 14, 2017 23:11
}

// Anything that starts with "${ " should be processed as a template arg.
if !strings.HasPrefix(v, "${") {
Copy link
Contributor

Choose a reason for hiding this comment

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

and ends with }?

Copy link
Contributor Author

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Updated to use the interpolate package, which addresses most of the feedback (it parses everything, including simple literals, and allows for concatenations like ${host}:${port}

@@ -0,0 +1,3 @@
service: foo
request:
name: ${user:foo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cover a lot of the edge cases such as \\ and \$ in TestProcessString and TestProcessMap already. but added a couple more to args.yaml -- it has a fallback which is not a string, a list replacement, and testing for escapes.

@@ -0,0 +1,8 @@
service: foo
headers:
header1: ${user:foo}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is not replaced at the moment, template arguments are used for request bodies.

Before 1.0, we may want to think about processing everything in the YAML file? We would probably decode the template to a map, use templateargs.ProcessMap, then pass it through mapdecode. Thoughts @kriskowal @abhinav

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. Although mapdecode expects to decode into a known struct shape, not dynamic Thrift shapes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we want to use it to load into the Template struct (known Go shape). Won't affect the body which is still a map.

Template arguments are of the format ${argname:fallback}. Arguments can
be specified on the commandline using `--arg` or `-A`. Arguments are
parsed as YAML values, and allow more complex types (e.g., lists) to be
specified as arguments.

We only use template arguments for request bodies currently.

Argument parsing is based on @abhinav's work for YARPC configuration.
options.go Outdated
@@ -52,6 +52,7 @@ type RequestOptions struct {
Health bool `long:"health" description:"Hit the health endpoint, Meta::health"`
Timeout timeMillisFlag `long:"timeout" default:"1s" description:"The timeout for each request. E.g., 100ms, 0.5s, 1s. If no unit is specified, milliseconds are assumed."`
YamlTemplate string `short:"y" long:"yaml-template" description:"Send a tchannel request specified by a yaml template"`
TemplateArgs map[string]string `short:"A" long:"arg" description:"A list of key-value template arguments"`
Copy link
Contributor

Choose a reason for hiding this comment

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

add example of key-value argument format to the help. Is it -A foo=bar or -A foo:bar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@prashantv prashantv merged commit 8da96d5 into dev Apr 7, 2017
@prashantv prashantv deleted the template_args branch April 7, 2017 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants