realestate-com-au / stackup

a simple CLI and Ruby API for AWS CloudFormation
MIT License
97 stars 34 forks source link

adjusting parse_body to use inbuilt gem json.parse instead of regex #41

Closed liomthechef closed 7 years ago

liomthechef commented 7 years ago

problems have been experienced when inputting yaml CFN templates into stackup, the logic within source.rb uses a regex match which seems to occasionally define valid yaml templates as json and breaks: /usr/local/lib/ruby/2.3.0/json/common.rb:156:in `parse': 822: unexpected token at '--- (MultiJson::ParseError)

We've repeated the issue across multiple yaml based CFN templates, let me know if you want a sample non publicly.

The fix proposed is to use the inbuilt json gem parser to attempt to parse the input. Ideally I wouldn't rely on an exception but I'm not sure of a more effective way.

mdub commented 7 years ago

Ah, bugger. I'm guessing this is related to #39?

I'm currently travelling, without access to a keyboard, so won't be able to give this proper attention until early May! Sorry.

I'd like to investigate whether just fixing the regex would help.

Failing that, we could go the exception-catching route. The code could be simplified, though, by just attempting the MultiJson.load and falling back to YAML on error.

liomthechef commented 7 years ago

Does look similar to #39 although I think our situation might be a bit more difficult to workaround without going json instead.

I'm not sure a regex change will be an easy fix, mostly because the inclusion of cfn-init config into templates means a lot of the typical json characters will be included in functions within the yaml format, which will make it a bit tricky to match json specific patterns (in our case anyway).

I've added a commit to use the simplified method you suggested and tested it successfully.

lukeck commented 7 years ago

👍 Looks good to me. I'm going to merge and release this.

Thanks @lnunns