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: pass all files in the array instead of just the first element #159

Closed
wants to merge 11 commits into from

Conversation

nkazarian-spokeo
Copy link
Contributor

@nkazarian-spokeo nkazarian-spokeo commented Nov 3, 2020

If there's more than one file that gets passed to the underlying hook script for tfsec, it does not get properly propagated. Pass the entire set instead.

Source

@nkazarian-spokeo
Copy link
Contributor Author

Any updates on this, please?

@MaxymVlasov MaxymVlasov added estimate/1h Need 1 hour to be done hook/terraform_tfsec Bash hook bug Something isn't working labels Sep 9, 2021
@MaxymVlasov MaxymVlasov added this to the Bug and docs fixes milestone Sep 9, 2021
@MaxymVlasov MaxymVlasov self-assigned this Sep 30, 2021
@MaxymVlasov MaxymVlasov self-requested a review September 30, 2021 20:17
Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

v1.51.0 (current) works the same as your improvements with require_serial: true flag

Without require_serial, as proposed in PR now, tf_sec generates identical issues many times.

So, need to perform tests run like this one, to find a better solution:

  • add a comment about "hack" to exiting code
    or
  • merge your improvements + require_serial: true flag

@MaxymVlasov
Copy link
Collaborator

I will run tests

@MaxymVlasov
Copy link
Collaborator

At the same time, if I run pre-commit try-repo -a antonbabenko/pre-commit-terraform terraform_tfsec it generates some mess as you wrote in the PR description.

Interesting.

@MaxymVlasov
Copy link
Collaborator

1000 runs terraform_tfsec PR #159 with require_serial: true:

time command max min mean median
users seconds 2.19 1.42 1.7872 1.79
system seconds 1.18 0.41 0.79795 0.8
CPU % 42 25 39.095 39
Total time 9.867 5.639 6.533213 6.5

1000 runs terraform_tfsec v1.51.0 (master) with try-repo:

time command max min mean median
users seconds 2.83 2.08 2.36483 2.36
system seconds 1.46 0.75 1.06849 1.07
CPU % 95 81 87.034 87
Total time 4.508 3.665 3.923429 3.922

1000 runs terraform_tfsec v1.51.0:

time command max min mean median
users seconds 1.72 1.02 1.3488 1.35
system seconds 1.17 0.49 0.8341 0.83
CPU % 36 33 34.571 35
Total time 6.677 5.975 6.226793 6.222

@MaxymVlasov
Copy link
Collaborator

Not sure what we should do with this PR. @antonbabenko ?

@antonbabenko
Copy link
Owner

I think this PR is not needed since #211 is merged and the terraform_tflint hook is executed in a serial way.

@MaxymVlasov MaxymVlasov closed this Oct 1, 2021
@MaxymVlasov
Copy link
Collaborator

Can fix #196 if add require_serial: true as in #237

@MaxymVlasov
Copy link
Collaborator

Hi @nkazarian-spokeo
Please, add require_serial: true as in #237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working estimate/1h Need 1 hour to be done hook/terraform_tfsec Bash hook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants