bazaarvoice / cloudformation-ruby-dsl

Ruby DSL for creating Cloudformation templates
Apache License 2.0
210 stars 76 forks source link

Adding a file read function that strips comments and empty lines. #111

Closed Noirbot closed 7 years ago

Noirbot commented 7 years ago

Description

Adds a function to read in a file while stripping out Bash and Ruby style comments, as well as empty lines. This helps keep templates more readable, while allowing you to comment files as needed in source.

I didn't add it to the usage examples, since it's functionally used the same as file(name), but I can if people feel that would be helpful.

Steps to Test or Reproduce

You should be able to reproduce this by using it to read in a file like so:

#This is a comment

curl google.com

and it should be delivered as:

curl google.com

Deploy Notes

This could require at least a patch version bump, since it's new functionality.

Related Issues and PRs

Todos

Request to Review

@jonaf/@temujin9

jonaf commented 7 years ago

How would you feel about incorporating this change into the --no-pretty option? I don't think it's good design to have a specialized function which has to be changed for development/production use-cases. The code should be the same, with different settings/arguments/environment. The --no-pretty option currently minifies the JSON to the extent possible. You could have it also strip bash comments.

Originally, --no-pretty was called --minify.

Noirbot commented 7 years ago

My issue there is that there's a difference between prettifying the template as a whole, and prettifying the user-data files that are being read in. In my current use case, I don't need/desire the comments to be in the template at all, in any environment, because it just clutters the template. Our UserData scripts right now are at least 50% comments and whitespace by line, so I just want it always stripped down.

That said, I could see the case for merging the two file-read functions into one, and giving it a boolean parameter for if you want it cleaned up, and then having the --no-pretty option force it on. But I think there should be an option to just always have your files read in without comments.

jonaf commented 7 years ago

In my current use case, I don't need/desire the comments to be in the template at all, in any environment, because it just clutters the template.

Have you considered removing the comments from your shell script entirely, then?

Noirbot commented 7 years ago

I want the comments to be in the shell script as it is committed in our version control, because the comments are valuable to anyone who needs to edit it in the future, or understand what we're installing or starting and why.

I don't need the comments to be sent to the instances themselves, or have them as part of the Cloudformation template, since it's highly unlikely that anyone would be trying to edit the startup script from inside the instance, or by editing the rendered template manually. It just becomes extra bytes to send around, and makes the template harder to read.

jonaf commented 7 years ago

Please pardon my ignorance, but I'm trying to understand exactly how this isn't minifying. I would argue that comments make the script easier to read, so --no-pretty makes sense in that context. If your motivation is to reduce the bytes that are being sent around, then you have exactly the same goal that was envisioned when the --no-pretty option was added.

Your counterargument to my suggestion to use this option is unclear to me. Can you help me understand this?

My issue there is that there's a difference between prettifying the template as a whole, and prettifying the user-data files that are being read in.

My interpretation is that you have a use-case where you want to strip the comments from the shell script but not minify the template?

temujin9 commented 7 years ago

Actually, I've got a use-case which is the other way around: I want to minify the template, but not strip commentary from shell scripts I'm including.

Frankly, I'm not real hot on making this part the CLI or the underlying DSL. It feels a lot more like a one-off need, rather than a common request, and thus one which is much better solved in client code than in the core library. I'm obviously open to counter-arguments about general viability, but this is the very first time I've seen this particular need in the wild.

Noirbot commented 7 years ago

I'm fine with making it a pre-script that I run on my own code if it doesn't seem like a useful tool for the general DSL.

My reason for wanting to minify the scripts, but not the template is that there's a case where I may need/want to go into cloudformation and see the template that was used - which resources were requested and with what settings, in an easy-to-read formatting - but I don't have a use case where examining the template would require checking the UserData section.

In short, having the template be pretty provides value to me. Having the UserData in the template preserve all the comments does not.

Either way, if this doesn't seem generally applicable, I'm fine just coding it into my project and moving on.