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

More powerful bindings parser #219

Merged
merged 1 commit into from
Dec 4, 2017
Merged

More powerful bindings parser #219

merged 1 commit into from
Dec 4, 2017

Conversation

fw42
Copy link
Contributor

@fw42 fw42 commented Dec 4, 2017

  • Fix a bug in the kubernetes-deploy --bindings parser. Previously, it was not possible to pass bindings for which the value contains commas (because the comma was interpreted as a separator). This is now possible via something like --bindings='"foo=bar,42",bla=42'.
  • Add possibility to pass a JSON encoded hash, e.g. --bindings='{ "foo": 42, "bar": 17 }'.
  • Add some tests for the parser.

Copy link

@pushrax pushrax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Using JSON will result in much cleaner types.

end

def parse_csv(string)
lines = CSV.parse(string)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be baseless but I'm not 100% certain that this will parse things that worked before (which just used split(',')). The most obvious example would be a newline in a value.

However, the additional quote parsing flexibility would be nice, so maybe the answer is "ship and see"

@ibawt
Copy link
Contributor

ibawt commented Dec 4, 2017

can you add some json to the integration tests for the bindings?

Copy link
Contributor

@ibawt ibawt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, but can you make sure this is run through the integration tests?

@fw42
Copy link
Contributor Author

fw42 commented Dec 4, 2017

Hm I don't see an integration test that exercises the exe/kubernetes-deploy script. Are there any? The integration tests I see are passing the bindings in as a hash (i.e. already parsed).

@ibawt
Copy link
Contributor

ibawt commented Dec 4, 2017

ok should be fine then. I thought for some reason we did it from the command.

You will have to super careful with the shell interpolations though.

@fw42 fw42 merged commit 363775c into master Dec 4, 2017
@fw42 fw42 deleted the bindings-parser branch December 4, 2017 17:26
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

Successfully merging this pull request may close these issues.

None yet

4 participants