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

fix: Pre-commit-terraform terraform_validate hook #401

Merged
merged 3 commits into from
Jun 21, 2022

Conversation

nshenry03
Copy link
Contributor

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

This PR simple redirects stdout and stderror to /dev/null for the cd command.

Fixes hooks/terraform_validate.sh

The terraform_validate hook was failing for me because the script was getting No such file or directory error:

hnicholas@hnicholas-a01:~/Repositories/terraform/modules/aws/kms$ pre-commit run -a
Terraform fmt............................................................Passed
Terraform validate.......................................................Failed
- hook id: terraform_validate
- exit code: 1

/Users/hnicholas/.cache/pre-commit/repo7ebw704n/hooks/terraform_validate.sh: line 114: pushd: $'/Users/hnicholas/Repositories/terraform/modules/aws/kms/examples/backend_account\n/Users/hnicholas/Repositories/terraform/modules/aws/kms/examples/backend_account': No such file or directory

Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform validate with tfsec............................................Passed
check for added large files..............................................Passed
check for merge conflicts................................................Passed
don't commit to branch...................................................Passed

Digging deeper, it seems that cd was outputting the name of the directory, but we don't want that, we only want output from pwd -P.

How can we test changes

I changed this locally and it fixed the issue.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
nshenry03 Nicholas Henry
@yermulnik
Copy link
Collaborator

@nshenry03 cd by default shouldn't output dirname it changed directory to.
Can you please share the output of type cd command?

@nshenry03
Copy link
Contributor Author

Thanks @yermulnik, here is the output from type cd:

$ type cd
cd is a shell builtin

It seems to for me

$ cd Repositories/terraform/modules/aws/kms/
/Users/hnicholas/Repositories/terraform/modules/aws/kms
$

System Info

$ system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: macOS 11.6.7 (20G630)
      Kernel Version: Darwin 20.6.0
      Boot Volume: Macintosh HD
      Boot Mode: Normal
      Computer Name: hnicholas-a01
      User Name: Nicholas Henry (hnicholas)
      Secure Virtual Memory: Enabled
      System Integrity Protection: Enabled
      Time since boot: 10 days 21:01

$ echo $SHELL
/usr/local/bin/bash

$ ll /usr/local/bin/bash
lrwxr-xr-x  1 hnicholas  admin  30 Jan 14 09:16 /usr/local/bin/bash@ -> ../Cellar/bash/5.1.16/bin/bash

$ brew info bash
bash: stable 5.1.16 (bottled), HEAD
Bourne-Again SHell, a UNIX command interpreter
https://www.gnu.org/software/bash/
/usr/local/Cellar/bash/5.1.16 (157 files, 10.9MB) *
  Poured from bottle on 2022-01-14 at 09:16:49
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/bash.rb
License: GPL-3.0-or-later
==> Options
--HEAD
        Install HEAD version
==> Analytics
install: 21,000 (30 days), 64,439 (90 days), 311,670 (365 days)
install-on-request: 16,991 (30 days), 52,064 (90 days), 256,604 (365 days)
build-error: 33 (30 days)

@yermulnik
Copy link
Collaborator

On the other hand shell scripts shouldn't be taking into account aliases and functions 🤔 What's your OS and Bash versions?

@nshenry03 nshenry03 changed the title Fix hooks/terraform_validate.sh fix: hooks/terraform_validate.sh Jun 21, 2022
@nshenry03 nshenry03 changed the title fix: hooks/terraform_validate.sh fix: Pre-commit-terraform terraform_validate hook Jun 21, 2022
@yermulnik
Copy link
Collaborator

Aha, that's because of CDPATH var:

 If a non-empty directory name from CDPATH is used, or if - is the first argument, and the directory change is successful, the absolute pathname of the new working directory is written to the standard output.

So we only need to suppress stdout. I'll leave a commit suggestion in a sec.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Only supress stdout and not stderror

Co-authored-by: George L. Yermulnik <[email protected]>
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

LGTM

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Add a space after `>`.

Co-authored-by: George L. Yermulnik <[email protected]>
@yermulnik
Copy link
Collaborator

yermulnik commented Jun 21, 2022

Once @MaxymVlasov and/or @antonbabenko make it to this PR to do the review, it will be merged.

@nshenry03 Thanks for contribution.

@antonbabenko antonbabenko merged commit d9f482c into antonbabenko:master Jun 21, 2022
antonbabenko pushed a commit that referenced this pull request Jun 21, 2022
## [1.72.2](v1.72.1...v1.72.2) (2022-06-21)

### Bug Fixes

* Pre-commit-terraform terraform_validate hook ([#401](#401)) ([d9f482c](d9f482c))
@antonbabenko
Copy link
Owner

This PR is included in version 1.72.2 🎉

@@ -111,7 +111,7 @@ function terraform_validate_ {

if [[ -n "$(find "$dir_path" -maxdepth 1 -name '*.tf' -print -quit)" ]]; then

pushd "$(cd "$dir_path" && pwd -P)" > /dev/null
pushd "$(cd "$dir_path" > /dev/null && pwd -P)" > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, you found code written in Paleolithic.

Let's try reuse logic that used in common functions here:

https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/_common.sh#L178

Absolutely have no idea why it was written in so complex way

nshenry03 added a commit to nshenry03/pre-commit-terraform that referenced this pull request Jun 30, 2022

Verified

This commit was signed with the committer’s verified signature. The key has expired.
nshenry03 Nicholas Henry
From @MaxymVlasov in antonbabenko#401 (comment):
> Heh, you found code written in Paleolithic.
>
> Let's try reuse logic that used in common functions here:
>
> https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/_common.sh#L178
>
> Absolutely have no idea why it was written in so complex way
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants