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

docs: Way we can skip directories #631

Merged
merged 4 commits into from
Feb 28, 2024
Merged

Conversation

Ak-sky
Copy link
Contributor

@Ak-sky Ak-sky commented Feb 20, 2024

way we can skip directories

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

How can we test changes

way we can skip directories
@Ak-sky Ak-sky changed the title Update README.md docs: way we can skip directories Feb 21, 2024
@Ak-sky Ak-sky changed the title docs: way we can skip directories docs: Way we can skip directories Feb 21, 2024
README.md Outdated
Comment on lines 863 to 866
- >
--args=--format json
--skip-dirs="**/.terragrunt-cache"
--args=--skip-dirs "**/.terraform"
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docs are right.

That's a multiline

You can rewrite

- >
           --args=--format json
           --skip-dirs="**/.terragrunt-cache"

as

- --args=--format json --skip-dirs="**/.terragrunt-cache"

I don't remember why there were problems, if any with more common for our hooks syntax

 - --args=--format=json
 - --args=--skip-dirs="**/.terraform"

Copy link
Contributor Author

@Ak-sky Ak-sky Feb 21, 2024

Choose a reason for hiding this comment

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

We are using it with pre-commit but this kept on giving errors unrecognized option below -

Terraform validate with trivy............................................Failed
- hook id: terraform_trivy
- exit code: 1

getopt: unrecognized option '--skip-dirs '\''**/.terraform'\''
'

Checkov..................................................................Passed

so we tried updating our pre-commit-config.yaml with below two configs and it worked fine.

- id: terraform_trivy
      files: '\.tf'
      args:
        - >
        - --args=--skip-dirs="**/.terraform" 
  - id: terraform_trivy
    files: '\.tf'
    args:
      - >
      - --args=--skip-files="**/.terraform/**/*"
      - --args=--skip-files="**/examples/**/*"
      - --args=--skip-files="**/tests/**/*"

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, you set everything as array, and it works. That's nice finding

You can remove

      - >

as it makes no sense in such construction

Copy link
Collaborator

@MaxymVlasov MaxymVlasov Feb 21, 2024

Choose a reason for hiding this comment

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

Please change docs to

args:
  - --args=--format=json
  - --args=--skip-dirs="**/.terraform"

Copy link
Collaborator

Choose a reason for hiding this comment

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

But on the other hand, have you tried

  - id: terraform_trivy
    files: '\.tf'
    args:
      - >
        --args=
          --skip-files="**/.terraform/**/*"
          --skip-files="**/examples/**/*"
          --skip-files="**/tests/**/*"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But on the other hand, have you tried

  - id: terraform_trivy
    files: '\.tf'
    args:
      - >
        --args=
          --skip-files="**/.terraform/**/*"
          --skip-files="**/examples/**/*"
          --skip-files="**/tests/**/*"

Yes this one is also working fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good.
Then, to maintain the same examples across docs, we can change that part to #631 (comment) (because current proposal not works)

And add a section about #631 (comment) somewhere in docs to describe how --args= can be set only once. That could be useful when many arguments should be populated to hook.
At the same time, the "less boiler-plate" configs do not support comments, so we can't do something like

    args:
      - >
        --args=
          --skip-files="**/.terraform/**/*"
          --skip-files="**/examples/**/*" # Foo  bar
          --skip-files="**/tests/**/*"

or

    args:
      - >
        --args=
          --skip-files="**/.terraform/**/*"
          # Foo  bar
          --skip-files="**/examples/**/*"
          --skip-files="**/tests/**/*"

We need to move them out for that:

    args:
      - --args=--skip-files="**/examples/**/*" # Foo  bar
      - >
        --args=
          --skip-files="**/.terraform/**/*"
          --skip-files="**/tests/**/*"

What do you think, will such section be useful, or better to stick everyone to the same config type and not deal with YAML tricks?
@Ak-sky @yermulnik

Copy link
Collaborator

@yermulnik yermulnik Feb 21, 2024

Choose a reason for hiding this comment

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

I liked the format from this comment (array of args): #631 (comment)
Multiline is cool, though is messy, especially if indentation isn't correct (that's what I missed from the initial change in this PR).

Most probably those who are good enough with YAML tricks would be able to re-format to multiline if needed. But for "ordinary mortals" I'd maintain as simple config layout as possible and as applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we good with this?

@MaxymVlasov MaxymVlasov reopened this Feb 21, 2024
@MaxymVlasov MaxymVlasov merged commit c29bdb1 into antonbabenko:master Feb 28, 2024
6 checks passed
@Ak-sky Ak-sky deleted the patch-1 branch February 29, 2024 09:39
@antonbabenko
Copy link
Owner

This PR is included in version 1.88.1 🎉

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.

4 participants