sailthru / relay.mesos

A mesos plugin for Relay that lets you auto-scale the number of currently running instances of a bash command
Apache License 2.0
38 stars 3 forks source link

Curly braces in cooler/warmer commands got interpolated #1

Closed akerekes closed 9 years ago

akerekes commented 9 years ago

If we launch realy.mesos with a bash command that contains curly braces, it gets interpolated by python while building the command. Should me more clearly documented this mechanism.

Workaround: add another curly braces around the original ones.

adgaudio commented 9 years ago

The problem is the following:

If you define this command,

relay.mesos --warmer "echo {abc}"

the python code in relay.mesos tries to fill in the variable between brackets using python's string interpolation, or abc=bar python -c 'import os ; print("foo {abc}".format(**os.environ))'. This is expected, and I previously thought preferable behavior because it's easy in this case to fetch environment variables at runtime and inject them into the command.

However, this can lead to unexpected consequences, particularly if you use "echo ${abc}". Another problem example would be If you define a warmer function with json code in it:

relay.mesos --warmer 'echo "{\"this_is_json\": 123}"'

python thinks the json code is string formatting, incorrectly tries to interpolate the characters between the brackets, and raises a KeyError.

Options:

  1. The simplest thing to do would be to remove the implicit string formatting because it's 1) non-obvious and 2) non-standard. This is backwards incompatible, though I don't think anyone except us uses relay.mesos at the moment. This is also just a one-line change.
  2. A different option to do would be to keep this behavior, document it, and expect users to learn about this behavior. This sounds hacky.
  3. Provide an option to enable or disable string formatting? I don't think this feature is useful enough to warrant it.

I think option 1 is probably best. Remove the "feature." The more I write about it the more I realize this is an aweful feature! :)

jdef commented 9 years ago

+1 remove the "feature" On Jul 20, 2015 6:20 PM, "Alex Gaudio" notifications@github.com wrote:

The problem is the following:

If you define this command,

relay.mesos --warmer "echo ${abc}"

the python code in relay.mesos tries to fill in the variable between brackets using python's string interpolation, or abc=bar python -c 'import os ; print("foo {abc}".format(**os.environ))'. This is expected, and I previously thought preferable behavior because it's easy in this case to fetch environment variables at runtime and inject them into the command.

However, this can lead to unexpected consequences. If you define a warmer function with json code in it:

relay.mesos --warmer 'echo "{\"this_is_json\": 123}"'

python thinks the json code is string formatting, incorrectly tries to interpolate the characters between the brackets, and raises a KeyError.

Options:

  1. The simplest thing to do would be to remove the implicit string formatting because it's 1) non-obvious and 2) non-standard. This is backwards incompatible, though I don't think anyone except us uses relay.mesos at the moment.
  2. A different option to do would be to keep this behavior, document it, and expect users to learn about this behavior. This sounds hacky.
  3. Provide an option to enable or disable string formatting? I don't think this feature is useful enough to warrant it.

I think option 1 is probably best. Remove the feature.

— Reply to this email directly or view it on GitHub https://github.com/sailthru/relay.mesos/issues/1#issuecomment-123070003.

akerekes commented 9 years ago

I'd make the default behavior to be non-interpolating and enable interpolation by a command line flag.