search

StackStorm-Exchange / stackstorm-ansible

st2 content pack containing ansible integrations
https://exchange.stackstorm.org/
Apache License 2.0
36 stars 28 forks source link

Add JSON support in extra_vars #7

Closed cognifloyd closed 7 years ago

cognifloyd commented 7 years ago

The docs[1] say that ansible supports json/yaml in extra_vars. Fixes #6. This adds support for sending arbitrarily complex extra_vars (such as when specifying parameters in a workflow) as json to ansible.

This also changes the concatenation of key=value pairs so that only contiguous key=value pairs are merged into the same extra-vars arg. That way, order is preserved in the rare case where someone overrides a variable in a subsequent extra_vars entry.

[1] http://docs.ansible.com/ansible/playbooks_variables.html#passing-variables-on-the-command-line

cognifloyd commented 7 years ago

I squashed my commits, fixed various doc strings, added an entry to CHANGES.md, and bumped the version to 0.5.0. I think this is ready to be merged.

arm4b commented 7 years ago

@chris-downs would be nice if you could take a look too, since you initiated adding advanced functionality for extra vars.

arm4b commented 7 years ago

Thanks for adding descriptions! :+1:

We still need some simple unit tests for the _parse_extra_vars method. With the increased complexity, we can't afford that luxury of not writing tests anymore.

cognifloyd commented 7 years ago

I have to learn about unit tests in python. I think there's a doc page about testing packs, so I'll dig into that later.

cognifloyd commented 7 years ago

There are now 14 tests for _parse_extra_vars, 7 of which were written to pass with the current master (of those 7, only one test needed to be changed to account for order-preservation of arguments in this version). The other 7 test the json functionality. I did not add tests for any other code paths.

Thanks to @nmaludy, @m4dcoder, and @lakshmi-kannan for helping me figure out how to put these tests together.

cognifloyd commented 7 years ago

I saw a simple whitespace issue in the readme, so I fixed it. This is ready to merge imo.