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

Curly braces in cooler/warmer commands got interpolated #1

Closed
akerekes opened this issue Jul 20, 2015 · 3 comments
Closed

Curly braces in cooler/warmer commands got interpolated #1

akerekes opened this issue Jul 20, 2015 · 3 comments

Comments

@akerekes
Copy link

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
Copy link
Contributor

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
Copy link

jdef commented Jul 21, 2015

+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
#1 (comment).

@akerekes
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants