Add ability to render the template config files. #271

sodre · 2017-01-24T05:00:25Z

This is useful in AutoPilot applications like MySQL that need to rewrite the containerpilot.conf file and force a SIGHUP. In particular, we could correctly address autopilotpattern/mysql#77.

You can use the code like this:

Rendering $CONTAINERPILOT to stdout containerpilot -render -
Rendering $CONTAINERPILOT to file containerpilot -render file:///etc/cp.conf
Rendering custom template to stdout containerpilot -config file:///etc/cp.tmpl -render -
Rendering custom template to file containerpilot -config file:///etc/cp.tmpl -render file:///etc/cp.conf

This is useful in AutoPilot applications like MySQL that need to rewrite the containerpilot.conf file and force a SIGHUP.

The unit tests now cast the runnable to core.App in order to pass.

tgross · 2017-01-24T13:36:04Z

As I noted in that MySQL thread, we are intentionally not rendering the entire ContainerPilot configuration. Do you have another example use case?

tgross · 2017-01-24T20:15:37Z

Ask, and a use case appears! #272

Is this use case something we could do with this PR?

sodre · 2017-01-25T02:39:49Z

Cool, @BobDickinson says that this patch would address #272. Let me know what joyent needs from my side to merge this PR.

tgross · 2017-01-25T14:19:38Z

config/render.go

+}
+
+// NewRenderApp creates a new Config Rendering application
+func NewRenderApp(configFlag string, renderFlag string) (*RenderApp, error) {


I would much rather see a Render method on Config than have this logic in a new interface around App. I see why you've created the new interface to begin with but it should delegate all the behavior to Config. The approach here ends up duplicating a bunch of code and that's going to be a source of bugs (like for example in this case we've left out all the template filter functions). Instead we should be calling ParseConfig to get a fully-parsed configuration that is identical to the one that the main application uses, and then making a Render call to that to dump it.

I suspect we should have this Render method dump a string (i.e. should have a signature like func (c *Config) Render() string) and then have the caller decide what to do with the string.

I'm also wondering whether we need to have a new interface on App at all. For the version flag we just did this right in the LoadApp and exited early: https://github.com/joyent/containerpilot/pull/271/files#diff-639ed475181e0558956e92333cd5e9c0R81

Oh actually maybe an even better signature would be func (c *Config) Render(io.Writer), so that the caller can pass in an arbitrary Writer for us to stream bytes into.

Starting with easy one, I will change the code to look more like the "-version", i.e. get rid of the "app" function. In that case, I just need to embed more of the logic in app.go.

On the issue of rendering the "config file" vs. "config struct": What I had in mind while writing the code is that after containerpilot -render is called you have a config file that can be used as input to containerpilot -config. For that to work correctly (I think) I only need to call 'ApplyTemplate' because all the additional work of creating the 'Config struct' will happen once containerpilot gets its SIGHUP and 'ParseConfig' gets called. The main idea here is to intentionally not create the struct and instead just render the template and obtain a valid JSON back.

I went through the ParseConfig file again and I did not see the "template filter functions" you talked about. Can you be more specific where they are located?

I went through the ParseConfig file again and I did not see the "template filter functions" you talked about. Can you be more specific where they are located?

I was talking about these but on second look you actually are passing thru those. So the approach you have after 06e0dc6 is fine but let's factor out the repeated code into functions

tgross · 2017-01-25T14:29:43Z

core/app.go

 	var versionFlag bool

 	if !flag.Parsed() {
 		flag.StringVar(&configFlag, "config", "",
 			"JSON config or file:// path to JSON config file.")
+		flag.StringVar(&renderFlag, "render", "",
+		  "- for stdout or file:// path where to save redenred JSON config file.")


I recognize that it would make the parsing harder, but maybe would it more more ergonomic to have the argument to -render be optional? So one could do:

$ containerpilot -render | whatever $ containerpilot -render -config file://containerpilot.json | whatever $ containerpilot -render file://myfile $ containerpilot -render file://myfile -config file://containerpilot.json

(I'd like to eliminate the file:// prefix requirement here too, but that'd break backwards compatibility with the existing config flag parsing so we're stuck with it until 3.x.)

Also, we've got a typo on the help string here.

I have no problems with the proposed syntax. The only difficulty I found, is that the "flag" module does not look like it cleanly supports what you are asking, I am new to Go so I could be wrong here.

My implementation to what you are asking would be the following:

... flag.StringVar(&renderFlag, "render", "/dev/null", ...) ... if renderFlag != "/dev/null" { // run the rendering code path... }

The issue is that if the user runs containerpilot -render /dev/null we will actually end up executing the regular containerpilot application.

If that is still okay I will make the change.

Sorry, 'flag' did not work as I thought it would. I am not sure how to implement what you asked without bigger surgery.

tgross · 2017-01-25T14:31:09Z

core/app_test.go

@@ -58,10 +58,11 @@ func TestValidConfigParse(t *testing.T) {

 	os.Setenv("TEST", "HELLO")
 	os.Args = []string{"this", "-config", testJSON, "/testdata/test.sh", "valid1", "--debug"}
-	app, err := LoadApp()
+	runnable, err := LoadApp()


We'll definitely need tests for this new feature. Some things to test:

passing the -render flag with valid arguments, invalid arguments, and without arguments

that the rendered output is as expected, including that all templating is being applied correctly

Completely agree, I was waiting for feedback before I spent more time on it.

deitch · 2017-01-26T08:23:51Z

Well, @sodre you definitely beat me to it. Your suggestion, and once the other autopilotpattern/mysql#77 PR was closed I was going to do this. I guess great minds think alike? :-)

Definitely in favour.

sodre · 2017-01-27T00:15:53Z

@tgross,
Following the suggestion from @deitch, what do you think about the following syntax?

$ containerpilot -template | whatever
$ containerpilot -template -config file://containerpilot.json | whatever
$ containerpilot -template -out file://myfile
$ containerpilot -template -out file://myfile -config file://containerpilot.json

I have that implemented in my local repo, if you like it, I can push it forward.

When ContainerPilot moves to 3.0, I would suggest using golang's FlagSet and use a subcommand "template" instead of a flag, e.g. containerpilot template -in <path> -out <path>

- containerpilot -template [-config file://] [-out file://]

tgross · 2017-01-27T13:41:20Z

what do you think about the following syntax?

Sure, let's go with that.

When ContainerPilot moves to 3.0, I would suggest using golang's FlagSet and use a subcommand "template" instead of a flag

Absolutely agreed on that.

tgross · 2017-01-27T14:05:19Z

core/app_test.go

+}
+
+func TestRenderConfigFileStdout(t *testing.T) {
+	// Because of the "exit(0)" in LoadApp, we need to use this testing pattern


This is a code smell that we haven't factored out the functionality enough.

tgross · 2017-01-27T18:07:50Z

Just FYI I'm going to cut a patch release (2.6.1) for some minor bug fixes, but this is getting into good enough shape that I think it'll be a good basis for the next minor version release (2.7.0).

sodre · 2017-01-31T04:06:11Z

That sounds good. Let me know what other things you would like changed.

tgross · 2017-01-31T15:45:51Z

I did a quick refactor to cover my comments here so once Travis gives us the thumbs up I think this is ready to merge.

deitch · 2017-01-31T15:53:17Z

(looking for a thumbs-up emoji to put on the "@tgross approved these changes", but no way to add a reaction to that....)

tgross · 2017-01-31T16:18:37Z

Released as https://github.com/joyent/containerpilot/releases/tag/2.7.0

Add RenderConfig option to ContainerPilot.

1ac5314

This is useful in AutoPilot applications like MySQL that need to rewrite the containerpilot.conf file and force a SIGHUP.

sodre changed the title ~~Add ability to render the template config files.~~ [WIP] Add ability to render the template config files. Jan 24, 2017

sodre added 2 commits January 24, 2017 00:25

LoadApp returns a Runnable interface.

5985ec6

The unit tests now cast the runnable to core.App in order to pass.

Make lint build happy.

6444caa

sodre force-pushed the f-template branch from 5ca3213 to 6444caa Compare January 24, 2017 05:38

tgross mentioned this pull request Jan 24, 2017

Expose containerpilot template processing #272

Closed

tgross suggested changes Jan 25, 2017

View reviewed changes

tgross changed the title ~~[WIP] Add ability to render the template config files.~~ Add ability to render the template config files. Jan 25, 2017

sodre added 2 commits January 25, 2017 22:11

Moved the render code into core/app.go

06e0dc6

Add unit tests for new -render function.

d1fdabe

sodre mentioned this pull request Jan 26, 2017

Add support for defining service name in env var autopilotpattern/mysql#77

Merged

Fixed lint issues and Regression test.

cdf7353

sodre force-pushed the f-template branch from b7341a6 to cdf7353 Compare January 26, 2017 07:21

Changed template rendering call syntax.

3bab056

- containerpilot -template [-config file://] [-out file://]

tgross reviewed Jan 27, 2017

View reviewed changes

Factor out config rendering code and move to config package

1c63b1f

tgross approved these changes Jan 31, 2017

View reviewed changes

tgross merged commit 46b0387 into TritonDataCenter:master Jan 31, 2017

sodre deleted the f-template branch February 1, 2017 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to render the template config files. #271

Add ability to render the template config files. #271

sodre commented Jan 24, 2017 •

edited

Loading

tgross commented Jan 24, 2017

tgross commented Jan 24, 2017

sodre commented Jan 25, 2017

tgross Jan 25, 2017

tgross Jan 25, 2017

sodre Jan 26, 2017

tgross Jan 27, 2017

tgross Jan 25, 2017

sodre Jan 26, 2017 •

edited

Loading

sodre Jan 26, 2017

tgross Jan 25, 2017

sodre Jan 26, 2017

deitch commented Jan 26, 2017

sodre commented Jan 27, 2017

tgross commented Jan 27, 2017

tgross Jan 27, 2017

tgross commented Jan 27, 2017

sodre commented Jan 31, 2017

tgross commented Jan 31, 2017

deitch commented Jan 31, 2017

tgross commented Jan 31, 2017

Add ability to render the template config files. #271

Add ability to render the template config files. #271

Conversation

sodre commented Jan 24, 2017 • edited Loading

tgross commented Jan 24, 2017

tgross commented Jan 24, 2017

sodre commented Jan 25, 2017

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

sodre Jan 26, 2017 • 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

deitch commented Jan 26, 2017

sodre commented Jan 27, 2017

tgross commented Jan 27, 2017

Choose a reason for hiding this comment

tgross commented Jan 27, 2017

sodre commented Jan 31, 2017

tgross commented Jan 31, 2017

deitch commented Jan 31, 2017

tgross commented Jan 31, 2017

sodre commented Jan 24, 2017 •

edited

Loading

sodre Jan 26, 2017 •

edited

Loading