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.

Sign up for GitHub

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

Jump to bottom

feat: Add terrafmt hook #313

Closed
wants to merge 15 commits into from
Closed

feat: Add terrafmt hook #313

wants to merge 15 commits into from

Conversation

rahulmlokurte
Copy link

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Adding Terrafmt to format terraform embedded in the document. Referencing #256

How has this code been tested

  1. Install terrafmt
  2. Add to .pre-commit-config.yaml:
- repo: https://github.com/rahulmlokurte/pre-commit-terraform
  rev: 90bd2a5727d2bbd5d5a27ed5390073f11c359a0c
  hooks:
    - id: terrafmt
      args:
        - --args=fmt
  1. Run
git add -A
pre-commit run

Example

@rahulmlokurte rahulmlokurte changed the title Terrafmt hook feat: Add terrafmt hook Jan 4, 2022
yermulnik
yermulnik requested changes Jan 5, 2022
View reviewed changes
Dockerfile
Comment on lines +130 to +135
RUN . /.env && \
if [ "TERRAFMT_VERSION" != "false" ]; then \
( \
curl -L "https://github.com/katbyte/terrafmt/archive/refs/tags/v0.3.0.zip" > terrafmt.zip \
) && unzip terrafmt.zip && rm terrafmt.zip \
; fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block must follow common for this file convention/standard for this kind of blocks:

  1. Have a comment line right before the block with the name of the tool.
  2. Have a logic to distinguish which version to download.

Moreover the katbyte/terrafmt repo does not distribute pre-compiled binaries, which means the {zip,tar}ball contains source code rather than the executable file. From what I see in katbyte/terrafmt README, the only available option to install the tool at the moment is to build it locally or use Golang builtin mechanism to build and install. Which doesn't seem to be an acceptable approach for pre-commit-terraform at the moment (@antonbabenko @MaxymVlasov ?).

.pre-commit-config.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
terrafmt.sh
@@ -0,0 +1,59 @@
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might worth to pick up what @MaxymVlasov works on in #310 to modernize and standardize common shell coding conventions and approaches across hook scripts in this project.

terrafmt.sh Outdated Show resolved Hide resolved
terrafmt.sh Outdated Show resolved Hide resolved
terrafmt.sh
}

terrafmt_() {
find . | grep -E "README.md" | sort | while read -r f; do
Copy link
Collaborator

@yermulnik yermulnik Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. find has an option to lookup files with particular names (or by a pattern), hence grep is extraneous and redundant here (as a side note, -E means grep should interpret pattern as an extended regex, which is redundant in this code construction).
  2. Hook config is configured in this PR to pass all markdown files to the hook (files: \.md$), so why would you need to pick only README.md files then?

@MaxymVlasov MaxymVlasov added estimate/4h Need 4 hours to be done feature New feature or request hook/terrafmt Bash hook labels Jan 5, 2022
MaxymVlasov
MaxymVlasov reviewed Jan 5, 2022
View reviewed changes
README.md
@@ -62,6 +62,7 @@ If you are using `pre-commit-terraform` already or want to support its developme
* [`TFSec`](https://github.com/liamg/tfsec) required for `terraform_tfsec` hook.
* [`infracost`](https://github.com/infracost/infracost) required for `infracost_breakdown` hook.
* [`jq`](https://github.com/stedolan/jq) required for `infracost_breakdown` hook.
* [`terrafmt`](https://github.com/katbyte/terrafmt) required for `terraform_fmt` hook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add needed installation instructions both for macOS and Ubuntu in the sections below.

rahulmlokurte and others added 12 commits January 5, 2022 20:35
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2022
@github-actions
Copy link

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Feb 20, 2022
@MaxymVlasov
Copy link
Collaborator

@rahulmlokurte are you working on that PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxymVlasov MaxymVlasov MaxymVlasov left review comments

@yermulnik yermulnik yermulnik requested changes

Assignees
No one assigned
Labels
estimate/4h Need 4 hours to be done feature New feature or request hook/terrafmt Bash hook stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rahulmlokurte @MaxymVlasov @yermulnik