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

Add a new terraform fmt command for linting #4955

Merged
merged 6 commits into from
Apr 4, 2016

Conversation

dcarley
Copy link
Contributor

@dcarley dcarley commented Feb 2, 2016

Add a new terraform fmt command for rewriting Terraform config files to a canonical format and style. This builds upon the hcl/fmtcmd package that was added in hashicorp/hcl#80 and wouldn't have been possible without @fatih's excellent work. There's more detail in the individual commits.

@dcarley
Copy link
Contributor Author

dcarley commented Feb 2, 2016

Looks like the dependencies need to be updated to pull in a later version of hashicorp/hcl.

How does this work, post #4909?

@dcarley dcarley force-pushed the add_fmt_cmd branch 2 times, most recently from 26f4b9b to d887d19 Compare February 2, 2016 22:10
@dcarley
Copy link
Contributor Author

dcarley commented Feb 2, 2016

Fixed the dependency problems 😋

@josephholsten
Copy link
Contributor

@dcarley looks great, and I was just looking at adding a -l to hclfmt this week.

I have a feeling this will bring fatih/hclfmt#3 to people's attention, since it's actually coming from hashicorp/hcl.

@phinze care to lay your 👀 on this one? Has tests, needs doc.

@dcarley dcarley force-pushed the add_fmt_cmd branch 2 times, most recently from d39687b to 9b6f0cd Compare February 8, 2016 09:18
@dcarley
Copy link
Contributor Author

dcarley commented Feb 8, 2016

I've rebased the existing commits to add website documentation. Sorry, forgot about that originally.

@dcarley
Copy link
Contributor Author

dcarley commented Mar 7, 2016

Rebased against master. I've omitted the update to the HCL dependency because #5400 pulled in a sufficient version.

@radeksimko @phinze: Can you review?

This uses the `fmtcmd` package which has recently been merged into HCL. Per
the usage text, this rewrites Terraform config files to their canonical
formatting and style.

Some notes about the implementation for this initial commit:

- all of the fmtcmd options are exposed as CLI flags
- it operates on all files that have a `.tf` suffix
- it currently only operates on the working directory and doesn't accept a
  directory argument, but I'll extend this in subsequent commits
- output is proxied through `cli.UiWriter` so that we write in the same way
  as other commands and we can capture the output during tests
- the test uses a very simple fixture just to ensure that it is working
  correctly end-to-end; the fmtcmd package has more exhaustive tests
- we have to write the fixture to a file in a temporary directory because it
  will be modified and for this reason it was easier to define the fixture
  contents as a raw string
The most common usage usage will be enabling the `-write` and `-list`
options so that files are updated in place and a list of any modified files
is printed. This matches the default behaviour of `go fmt` (not `gofmt`). So
enable these options by default.

This does mean that you will have to explicitly disable these if you want to
generate valid patches, e.g. `terraform fmt -diff -write=false -list=false`
So that you can operate on files in a directory other than your current
working directory.
So that you can do automatic formatting from an editor. You probably want to
disable the `-write` and `-list` options so that you just get the
re-formatted content, e.g.

    cat main.tf | terraform fmt -write=false -list=false -

I've added a non-exported field called `input` so that we can override this
for the tests. If not specified, like in `commands.go`, then it will default
to `os.Stdin` which works on the command line.
These options don't make sense when passing STDIN. `-write` will raise an
error because there is no file to write to. `-list` will always say
`<standard input>`. So disable whenever using STDIN, making the command
much simpler:

    cat main.tf | terraform fmt -
To ensure that these are passed over to `fmtcmd` correctly.
@dcarley
Copy link
Contributor Author

dcarley commented Mar 7, 2016

Oops, fixed build failure.

@ronnylt
Copy link

ronnylt commented Apr 2, 2016

It would be great to have this in terraform, looking forward to adding it to our CI process.

@josephholsten
Copy link
Contributor

@radeksimko @phinze: what do we need to get this in for 0.6.15? I'd actually love to have this command for every hashi-app using hcl, but this is where I'm needing it most right now.

@apparentlymart apparentlymart merged commit d883b76 into hashicorp:master Apr 4, 2016
apparentlymart added a commit that referenced this pull request Apr 4, 2016
@apparentlymart
Copy link
Contributor

Thanks for this, @dcarley!

I tried it out on some config files I have here and it worked out great. I've merged this set of changes for inclusion in 0.6.15.

@dcarley
Copy link
Contributor Author

dcarley commented Apr 4, 2016

Thanks! 🐳

@ghost
Copy link

ghost commented Apr 26, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants