boltops-tools / terraspace

Terraspace: The Terraform Framework
https://terraspace.cloud
Apache License 2.0
674 stars 46 forks source link

Implement `--noop` for `terraspace fmt` #316

Closed alexjfisher closed 11 months ago

alexjfisher commented 1 year ago

This is a 🙋‍♂️ feature or enhancement.

Summary

When the --noop option is used with terraspace fmt, return non-zero if one or more files are needing to be formatted, (but leave the files untouched). Useful feature for CI/CD pipelines.

Context

The --noop option seems to be common to all terraspace cli commands, but not implemented for all of them. For fmt we can use Terraform's -check option.

How to Test

Modify some stack/module tf files such that they have formatting issues. Run terraspace fmt --noop and note the return code and the fact files haven't been updated with the formatting fixes.

Version Changes

Minor?

alexjfisher commented 1 year ago

@tongueroo Hi! Is there anything else you'd like to see in this to get it over the line? Thanks.

tongueroo commented 1 year ago

Nope. Thanks for the detailed report. Wondering if should also just remove the noop option. Will consider either. Thanks.

alexjfisher commented 1 year ago

My initial thought was when people are running it in CI they'll want to run with --noop, but otherwise they'd want changes to be made.

But, now I've thought about it a bit more, if terraspace fmt is fixing the .terraspace-cache version then I don't suppose it serves any purpose. Have I understood that correctly?

tongueroo commented 1 year ago

So terraspace fmt demo does a little trickery to achieve formatting the files in the desired app/stacks/demo folder. It works on thr app files directly instead of the .terraspace-cache folder. There are some limits to it though, it will only consider a source file and format it if does not contain any ERB: https://github.com/boltops-tools/terraspace/blob/3d650171ce851ba0ebb0cef3a65112e97a7adcfb/lib/terraspace/cli/fmt/runner.rb#L28

alexjfisher commented 1 year ago

Ah, ok. So the change is probably ok as is?

tongueroo commented 11 months ago

Thanks so much for the PR.

Decided to generalize it a bit more in the #332 These all work now

terraspace fmt demo -check
terraspace fmt demo -write=false -list
terraspace fmt -write=false -list

And also remove the unused --noop and --verbose options in #333 There's a noop in one small case, and just change it to an env var for now.