Add the possibility to template playbooks #103

dannymc129 · 2017-07-11T19:48:16Z

I was recently recommended by @lritter to consider using this tool for some adhoc queries we need to run regularly and I ended up really liking it. Simple, clean, functional, we're already snowplow users and a perfect match.

But I wasn't happy with the idea of all of my hosts and password being hosted along with the playbooks, so to fix this I changed it so that the -var flag will pass into a playbook similar to the way it does for sql templates, then we can get our passwords out of local config without worrying the tool.

I also found out that based on:
https://github.com/snowplow/sql-runner/blob/master/sql_runner/options.go#L28-L36

there's a limit of only a single key=value pair in the -var flag, I extended this so that it will load any number of vars via comma separation (key=value,key2=value2)

Pretty new to go, please do pick it apart.

…ybook

snowplowcla · 2017-07-11T22:43:27Z

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://github.com/snowplow/snowplow/wiki/CLA to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

BenFradet

done a first pass, I think this is good use case 👍

BenFradet · 2017-07-12T11:42:01Z

sql_runner/consul_provider.go

@@ -15,12 +15,14 @@ package main
 type ConsulPlaybookProvider struct {
 	consulAddress string
 	consulKey     string
+	variables 		CLIVariables


the indent seems off

BenFradet · 2017-07-12T11:42:31Z

sql_runner/consul_provider.go

-		consulKey:     consulKey,
+		consulAddress: options.consul,
+		consulKey:     options.playbook,
+		variables: 		 options.variables,


same, don't hesitate to run gofmt

BenFradet · 2017-07-12T11:43:14Z

sql_runner/consul_provider.go

 }

-func NewConsulPlaybookProvider(consulAddress, consulKey string) *ConsulPlaybookProvider {
+func NewConsulPlaybookProvider(options Options) *ConsulPlaybookProvider {


are we sure we want to pass the whole options bundle and not just the address, key and variables?

BenFradet · 2017-07-12T11:43:50Z

sql_runner/yaml_provider.go

 }

-func NewYAMLFilePlaybookProvider(playbookPath string) *YAMLFilePlaybookProvider {
+func NewYAMLFilePlaybookProvider(options Options) *YAMLFilePlaybookProvider {


same remark as above

BenFradet · 2017-07-12T11:45:26Z

sql_runner/yaml_utils.go

@@ -26,13 +27,18 @@ var (

 // Parses a playbook.yml to return the targets
 // to execute against and the steps to execute
-func parsePlaybookYaml(playbookBytes []byte) (Playbook, error) {
+func parsePlaybookYaml(playbookBytes []byte, variables CLIVariables) (Playbook, error) {


might be worth it to restrict ourselves to just a map[string]string

BenFradet · 2017-07-12T11:46:30Z

sql_runner/yaml_utils.go

+		return "", err
+	}
+
+	return string(filled.String()), err


why the wrapping of a string in another string?

BenFradet · 2017-07-12T11:46:55Z

sql_runner/yaml_utils.go

@@ -51,3 +57,17 @@ func cleanYaml(rawYaml []byte) []byte {
 	}
 	return buffer.Bytes()
 }
+
+func fillPlaybookTemplate(playbookStr string, variables CLIVariables) (string, error) {


same I think we can drop CLIVariables in favour of map[string]string

coveralls · 2017-07-12T14:14:02Z

Coverage increased (+0.7%) to 33.247% when pulling bd704b2 on dannymc129:master into 0ae72da on snowplow:master.

BenFradet · 2017-07-13T08:50:13Z

sql_runner/options.go

+	for value := range split {
+		kv := strings.SplitN(split[value], "=", 2)
+
+		(*i)[kv[0]] = kv[1]


can we check the length of kv and send an error accordingly to avoid index out of range?

dannymc129 · 2017-08-04T14:47:34Z

I signed it!

coveralls · 2017-08-04T15:14:47Z

Coverage increased (+0.6%) to 33.161% when pulling e00c717 on dannymc129:master into 0ae72da on snowplow:master.

snowplowcla · 2017-08-04T15:41:08Z

Confirmed! @dannymc129 has signed the Individual Contributor License Agreement. Thanks so much

BenFradet

Looks great! 👍 , scheduling for the next version

BenFradet · 2018-01-10T16:49:44Z

Merging, sorry for the delay 👍

alexanderdean · 2018-01-10T18:16:04Z

@BenFradet I think this should have been split into two commits - one to add the new templating functionality, and the other to fix the limitation around the single variable provision.

BenFradet · 2018-01-10T18:23:54Z

Nw, we can still split the commit when merging to master.

I'll log the other issue.

alexanderdean · 2018-01-10T18:24:09Z

Thanks @BenFradet !

dannymc129 added 3 commits July 11, 2017 15:42

alter options to allow multiple vars by comma separation

e2da7ce

run playbook generator through template engine to get cli vars in pla…

be0f2c5

…ybook

test cli vars and playbook vars

6bfca22

snowplowcla added the cla: no label Jul 11, 2017

BenFradet reviewed Jul 12, 2017

View reviewed changes

dannymc129 added 2 commits July 12, 2017 10:11

feedback

f9cc291

missed a last cli

bd704b2

snowplow deleted a comment from coveralls Jul 12, 2017

BenFradet reviewed Jul 13, 2017

View reviewed changes

check length and return error

e00c717

snowplowcla added cla: yes and removed cla: no labels Aug 4, 2017

BenFradet approved these changes Aug 10, 2017

View reviewed changes

BenFradet added this to the Version 0.6.0 milestone Aug 10, 2017

tclass mentioned this pull request Dec 6, 2017

Make database password templatable #78

Closed

alexanderdean modified the milestones: Version 0.6.0, Version 0.7.0 Dec 24, 2017

BenFradet changed the base branch from master to develop January 10, 2018 16:44

BenFradet changed the title ~~Add Templating Options for Playbooks~~ Add the possibility to template playbooks Jan 10, 2018

BenFradet merged commit 23d1c08 into snowplow:develop Jan 10, 2018

BenFradet mentioned this pull request Jan 10, 2018

Allow passing multiple key value pairs to the -var flag #120

Closed

alexanderdean assigned BenFradet and mhadam Jun 10, 2018

mhadam pushed a commit that referenced this pull request Jul 5, 2018

Add the possibility to template playbooks (close #103)

8dfa229

mhadam pushed a commit that referenced this pull request Jul 5, 2018

Add the possibility to template playbooks (close #103)

d3b7eda

mhadam pushed a commit that referenced this pull request Jul 6, 2018

Add the possibility to template playbooks (close #103)

f81bbca

mhadam pushed a commit that referenced this pull request Jul 9, 2018

Add the possibility to template playbooks (close #103)

a2e8515

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the possibility to template playbooks #103

Add the possibility to template playbooks #103

dannymc129 commented Jul 11, 2017

snowplowcla commented Jul 11, 2017

BenFradet left a comment

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

BenFradet Jul 12, 2017

coveralls commented Jul 12, 2017 •

edited

Loading

BenFradet Jul 13, 2017

dannymc129 commented Aug 4, 2017

coveralls commented Aug 4, 2017 •

edited

Loading

snowplowcla commented Aug 4, 2017

BenFradet left a comment

BenFradet commented Jan 10, 2018

alexanderdean commented Jan 10, 2018

BenFradet commented Jan 10, 2018

alexanderdean commented Jan 10, 2018

Add the possibility to template playbooks #103

Add the possibility to template playbooks #103

Conversation

dannymc129 commented Jul 11, 2017

snowplowcla commented Jul 11, 2017

BenFradet left a 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

coveralls commented Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

dannymc129 commented Aug 4, 2017

coveralls commented Aug 4, 2017 • edited Loading

snowplowcla commented Aug 4, 2017

BenFradet left a comment

Choose a reason for hiding this comment

BenFradet commented Jan 10, 2018

alexanderdean commented Jan 10, 2018

BenFradet commented Jan 10, 2018

alexanderdean commented Jan 10, 2018

coveralls commented Jul 12, 2017 •

edited

Loading

coveralls commented Aug 4, 2017 •

edited

Loading