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

Enable terraform validate pre-commit hook #90

Merged
merged 4 commits into from
Sep 13, 2021

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented Aug 26, 2021

🗣 Description

This PR enables the terraform validate pre-commit hook.

💭 Motivation and context

That pre-commit hook had been disabled in all of our repositories because of issues related to proxy providers and Terraform code that uses remote state. We believe that those issues (see hashicorp/terraform#24887 and hashicorp/terraform#24896) have been corrected in Terraform 0.13.x, which we are in the process of migrating to, so this hook should now work as expected in most, if not all of our repos.

This PR will remain blocked until the following conditions have been met:

  • All of our repositories that use Terraform have been successfully updated to use Terraform 0.13.x.
  • All of our deployed Terraform states have been updated to Terraform 0.13.x. It is important to note that we cannot update any Terraform states to 0.14.x until they successfully apply with 0.13.x first.
  • Update to Terraform 0.13 setup-env-github-action#37 has been approved and merged.

This PR is a part of cisagov/cool-system#94.

Similar PRs are:

🧪 Testing

I verified that running terraform validate with Terraform 0.13.7 works without any errors in this repository. I also confirmed that all pre-commit hooks (including the newly-enabled terraform validate hook) run successfully.

After I confirmed that pre-commit with Terraform 0.13.7 ran successfully in GitHub Actions via 6a7fbf0, I reverted that commit (see b51dbb5) in preparation for when this PR becomes unblocked at the end of downstream testing.

Further testing will be done in all downstream repositories.

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

This hook should now work as expected in most, if not all of our repos 
now that we are finally updating to terraform 0.13.x (on our way to 
1.0.x).
@dav3r dav3r added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Aug 26, 2021
@dav3r dav3r self-assigned this Aug 26, 2021
…f_0.13

This change will be reverted when testing is completed.
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

I look forward to the day when terraform validate is again looking out for us.

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

This makes sense. I think we should remove

- name: Find and initialize Terraform directories
run: |
for path in $(find . -not \( -type d -name ".terraform" -prune \) \
-type f -iname "*.tf" -exec dirname "{}" \; | sort -u); do \
echo "Initializing '$path'..."; \
terraform init -input=false -backend=false "$path"; \
done

from the workflow because this is handled by the terraform_validate hook per these lines.

Initialization will now be done during the "terraform validate" step.
@dav3r
Copy link
Member Author

dav3r commented Aug 27, 2021

This makes sense. I think we should remove

- name: Find and initialize Terraform directories
run: |
for path in $(find . -not \( -type d -name ".terraform" -prune \) \
-type f -iname "*.tf" -exec dirname "{}" \; | sort -u); do \
echo "Initializing '$path'..."; \
terraform init -input=false -backend=false "$path"; \
done

from the workflow because this is handled by the terraform_validate hook per these lines.

Good call- I had forgotten about that! See 895a692.

@dav3r dav3r requested a review from mcdonnnj August 27, 2021 17:07
@dav3r dav3r added the blocked This issue or pull request is awaiting the outcome of another issue or pull request label Aug 27, 2021
@dav3r
Copy link
Member Author

dav3r commented Sep 11, 2021

@mcdonnnj - please review this when you can.

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Since it doesn't seem to barf in this branch, do we want to add

  - package-ecosystem: "terraform"
    directory: "/"
    schedule:
      interval: "weekly"

to .github/dependabot.yml since we are hitting the minimum version of Terraform needed to support Dependabot alerts? This would resolve cisagov/skeleton-tf-module#59 downstream and also cover (with modification of the directory when doing PRs) the same need in cisagov/skeleton-packer and cisagov/skeleton-ansible-role-with-test-user.

@dav3r
Copy link
Member Author

dav3r commented Sep 13, 2021

Since it doesn't seem to barf in this branch, do we want to add

  - package-ecosystem: "terraform"
    directory: "/"
    schedule:
      interval: "weekly"

to .github/dependabot.yml since we are hitting the minimum version of Terraform needed to support Dependabot alerts? This would resolve cisagov/skeleton-tf-module#59 downstream and also cover (with modification of the directory when doing PRs) the same need in cisagov/skeleton-packer and cisagov/skeleton-ansible-role-with-test-user.

My gut says to push that dependabot change to an upcoming Kraken and separate it from this TF 0.13 business. What do you think?

@mcdonnnj
Copy link
Member

Since it doesn't seem to barf in this branch, do we want to add

  - package-ecosystem: "terraform"
    directory: "/"
    schedule:
      interval: "weekly"

to .github/dependabot.yml since we are hitting the minimum version of Terraform needed to support Dependabot alerts? This would resolve cisagov/skeleton-tf-module#59 downstream and also cover (with modification of the directory when doing PRs) the same need in cisagov/skeleton-packer and cisagov/skeleton-ansible-role-with-test-user.

My gut says to push that dependabot change to an upcoming Kraken and separate it from this TF 0.13 business. What do you think?

I'm ok with that. I only mentioned it because I would also be willing to consider it part of Terraform 0.13 jazz.

@dav3r
Copy link
Member Author

dav3r commented Sep 13, 2021

My gut says to push that dependabot change to an upcoming Kraken and separate it from this TF 0.13 business. What do you think?

I'm ok with that. I only mentioned it because I would also be willing to consider it part of Terraform 0.13 jazz.

👍 Please reference this PR when you create a new PR with that dependabot change and we will decide when it makes the most sense to roll that one out.

@dav3r dav3r merged commit 1b5cd25 into develop Sep 13, 2021
@dav3r dav3r deleted the improvement/enable_terraform_validate branch September 13, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or pull request is awaiting the outcome of another issue or pull request improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants