brooklyncentral / brooklyn

This project has moved and is now part of the ASF
https://github.com/apache/incubator-brooklyn
72 stars 27 forks source link

New config keys for optional custom CHECK_RUNNING and STOP commands #1421

Closed alasdairhodge closed 10 years ago

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2351 SUCCESS This pull request looks good (what's this?)

ahgittin commented 10 years ago

nice and simple. re integration tests, maybe use the silly nc server example with scripts to record the pid somewhere brooklyn doesn't know about?

what would it take to add templating support? might be very simple, using TemplateProcessor.processTemplateContents, and would add a lot, as we could then use attributeWhenReady in config keys, and reference the config keys in the scripts. (mainly useful for launch i expect -- but very useful there, to stitch things together!)

alasdairhodge commented 10 years ago

+1 for the attributeWhenReady use-case! Have gone with the super-simple TemplateProcessor.processTemplateContents option you describe.

grkvlt commented 10 years ago

@ahgittin @alasdairhodge -1 on the addition of templating

Templating isn't going to work usefully here, because the commands are usually relative paths into the extracted archive, so their contents will not be modified. Additionally, because $ has special meaning for the shell, any variable access (such as $! in the example given for the launch command) will require escaping with [noparse]$![/noparse] which is horrible.

To do templating properly would require specifying the start, stop and is-running scripts as external URLs that could be processed, rather than as relative paths to files inside the archive. We could download and extract the archive locally, pull out the files, filter them and copy them separately, or grab the files from the remote machine once extracted there, parse and then copy them back, but both these approaches seem unsatisfactory, particularly in light of the fact that we don't know if the command is a script or a short Bash command sequence.

My preference is instead to have three more config keys, called _START_SCRIPTURL, _STOP_SCRIPTURL and _IS_RUNNING_SCRIPTURL, that are used in preference to the _...COMMAND keys if present. These URLs would be retrieved, parsed and copied to the remote server, and executed at the appropriate time. The original _...COMMAND keys can then be changed back to being executed verbatim as Bash commands that may or may not reference files from the installed software archive.

alasdairhodge commented 10 years ago

Interesting comments, @grkvlt; is this what you had in mind, @ahgittin?

I can imagine some very simple cases where it'd be useful to have interpolation available in a basic bash command (e.g. a port number to ping in the check-running script) but agree that it's more limited that what can be done by processing entire (remote) scripts.

ahgittin commented 10 years ago

The URL question is orthogonal to the messy templating. I agree tenplating won't work. I looked in to making freemarker gracefully ignore things it does recognize but that proves very hard. And I don't think we want to use a half-baked custom twmplating scheme.

Maybe the answer is to have a way specify parameters or env vars to pass?

And perhaps should handle that as a new PR?

Best Alex On 28 May 2014 16:28, "Alasdair Hodge" notifications@github.com wrote:

Interesting comments, @grkvlt https://github.com/grkvlt; is this what you had in mind, @ahgittin https://github.com/ahgittin?

I can imagine some very simple cases where it'd be useful to have interpolation available in a basic bash command (e.g. a port number to ping in the check-running script) but agree that it's more limited that what can be done by processing entire (remote) scripts.

— Reply to this email directly or view it on GitHubhttps://github.com/brooklyncentral/brooklyn/pull/1421#issuecomment-44421947 .

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2355 SUCCESS This pull request looks good (what's this?)

grkvlt commented 10 years ago

@ahgittin +1 @alasdairhodge I think separate pull request for templating and revert adc0340 and a465551

ahgittin commented 10 years ago

@alasdairhodge can you do the revert and add to the vanilla-software-blueprint.yaml example and corresponding VanillaSoftwareYamlTest.java. also update creating-yaml.md to mention these flags and mention the existence of the vanilla-software-blueprint.yaml example?

and let's think on templating v passing parameters, maybe discuss on IRC or IRL tomorrow!

alasdairhodge commented 10 years ago

Good suggestions; on it now.

alasdairhodge commented 10 years ago

Have rebased the unnecessary commits into oblivion, updated the YAML test and example to specify custom commands and shell env.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2380 FAILURE Looks like there's a problem with this pull request (what's this?)

grkvlt commented 10 years ago

Failure looks unrelated, I think this can be merged :frog:

ahgittin commented 10 years ago

really nice. a refresh of creating-yaml.md would be nice at some point ....