cisagov / pre-commit-packer

Provides pre-commit hooks for Packer projects.
Creative Commons Zero v1.0 Universal
15 stars 11 forks source link

packer_fmt says there are no files to check when file exists #24

Open nandac opened 2 years ago

nandac commented 2 years ago

🐛 Summary

When running the packer_fmt hook the message I receive is: Packer Format........................................(no files to check)Skipped

Even though I have .pkr.hcl file in my repository

To reproduce

Steps to reproduce the behavior:

  1. Create a .pre-commit-config.yml something like this:

    repos:
    - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.2.0
    hooks:
      - id: pretty-format-json
        args: ["--autofix", "--no-sort-keys", "--indent=2"]
      - id: fix-byte-order-marker
      - id: check-added-large-files
        args: ["--maxkb=500"]
      - id: check-case-conflict
      - id: check-executables-have-shebangs
      - id: check-symlinks
      - id: check-merge-conflict
      - id: detect-private-key
      - id: detect-aws-credentials
        args: ["--allow-missing-credentials"]
      - id: trailing-whitespace
    
    - repo: https://github.com/cisagov/pre-commit-packer
    rev: v0.0.2
    hooks:
      - id: packer_validate
      - id: packer_fmt
  2. Create some dummy config file with the .pkr.hcl extension.
    packer {
    required_plugins {
    amazon = {
      version = ">= 0.0.2"
      source  = "github.com/hashicorp/amazon"
    }
    }
    }

Expected behavior

I should not receive the no files to check message.

Any helpful log output or screenshots

Paste the results here: When running the command pre-commit run --all-files --color always I get the following output:

pretty format json...................................(no files to check)Skipped
fix utf-8 byte order marker..............................................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check for broken symlinks............................(no files to check)Skipped
check for merge conflicts................................................Passed
detect private key.......................................................Passed
detect aws credentials...................................................Passed
trim trailing whitespace.................................................Passed
Packer Validate..........................................................Passed
Packer Format........................................(no files to check)Skipped
jsf9k commented 2 years ago

Thanks for the issue @nandac!

Are you running pre-commit via pre-commit run -a or is it being run automatically via git commit? If the latter, it is important to note that git commit runs pre-commit only against the files that the commit changes.

jsf9k commented 2 years ago

@mcdonnnj, is there possibly a typo here? https://github.com/cisagov/pre-commit-packer/blob/aea3624d37f32ce5dd4fd6698c16844017d74895/.pre-commit-hooks.yaml#L7

Should that instead be this?

files: (packer\.json|\.pkr|\.hcl)$
nandac commented 2 years ago

Thanks, @jsf9k for your reply.

I think you are right in saying that I probably tried it through git commit instead of trying it just with the raw command.

I now have a different issue regarding getting it to work with packer_fmt's arguments such as -write or -recursive which come up with the following message even if the file is formatted correctly.

Packer Format............................................................Failed
- hook id: packer_fmt
- exit code: 1

Usage: packer fmt [options] [TEMPLATE]

  Rewrites all Packer configuration files to a canonical format. Both
  configuration files (.pkr.hcl) and variable files (.pkrvars.hcl) are updated.
  JSON files (.json) are not modified.

  If TEMPLATE is "." the current directory will be used.
  If TEMPLATE is "-" then content will be read from STDIN.

  The given content must be in Packer's HCL2 configuration language; JSON is
  not supported.

Options:
  -check        Check if the input is formatted. Exit status will be 0 if all
                 input is properly formatted and non-zero otherwise.

  -diff         Display diffs of formatting change

  -write=false  Don't write to source files
                (always disabled if using -check)

  -recursive     Also process files in subdirectories. By default, only the
                 given directory (or current directory) is processed.

Failed path: -recursive
================================

make: *** [validate] Error 1

This is how I am specifying it in my pre-commit-config.yaml

repos:
  - repo: https://github.com/cisagov/pre-commit-packer
    rev: v0.0.2
    hooks:
      - id: packer_fmt
        args: ["-write=true"]

I can see why write would fail because you have set it to check by default but recursive should work and I think it is because arguments are not handled and I believe that -write and -recursive are interpreted as being files, not arguments.

This could be fixed by checking if the beginning of the elements in $@ starts with a - character and passing it in.

We can handle the default case of check with no arguments as well.

If I were submit a fix would you be able to review and release?

mcdonnnj commented 2 years ago

@mcdonnnj, is there possibly a typo here?

https://github.com/cisagov/pre-commit-packer/blob/aea3624d37f32ce5dd4fd6698c16844017d74895/.pre-commit-hooks.yaml#L7

Should that instead be this?

files: (packer\.json|\.pkr|\.hcl)$

Packer HCL2 files are explicitly .pkr.hcl. The HCL compatible JSON files are .pkr.json, and we match the hard-coded packer.json for the older, non-HCL format.

mcdonnnj commented 2 years ago

Thanks, @jsf9k for your reply.

I think you are right in saying that I probably tried it through git commit instead of trying it just with the raw command.

I now have a different issue regarding getting it to work with packer_fmt's arguments such as -write or -recursive which come up with the following message even if the file is formatted correctly.

Packer Format............................................................Failed
- hook id: packer_fmt
- exit code: 1

Usage: packer fmt [options] [TEMPLATE]

  Rewrites all Packer configuration files to a canonical format. Both
  configuration files (.pkr.hcl) and variable files (.pkrvars.hcl) are updated.
  JSON files (.json) are not modified.

  If TEMPLATE is "." the current directory will be used.
  If TEMPLATE is "-" then content will be read from STDIN.

  The given content must be in Packer's HCL2 configuration language; JSON is
  not supported.

Options:
  -check        Check if the input is formatted. Exit status will be 0 if all
                 input is properly formatted and non-zero otherwise.

  -diff         Display diffs of formatting change

  -write=false  Don't write to source files
                (always disabled if using -check)

  -recursive     Also process files in subdirectories. By default, only the
                 given directory (or current directory) is processed.

Failed path: -recursive
================================

make: *** [validate] Error 1

This is how I am specifying it in my pre-commit-config.yaml

repos:
  - repo: https://github.com/cisagov/pre-commit-packer
    rev: v0.0.2
    hooks:
      - id: packer_fmt
        args: ["-write=true"]

I can see why write would fail because you have set it to check by default but recursive should work and I think it is because arguments are not handled and I believe that -write and -recursive are interpreted as being files, not arguments.

This could be fixed by checking if the beginning of the elements in $@ starts with a - character and passing it in.

We can handle the default case of check with no arguments as well.

If I were submit a fix would you be able to review and release?

@nandac Yes this issue is part of a series of enhancements for this project that have been stuck in my backlog.

nandac commented 2 years ago

@jsf9k Maybe I could take a look into a fix if I have time over the weekend. I am not an expert shell scripter but I have some ideas I would like to try out.

jsf9k commented 2 years ago

Please go ahead, @nandac! We'll be happy to review your PR when you're ready.

nandac commented 2 years ago

@jsf9k I have added this PR https://github.com/cisagov/pre-commit-packer/pull/25 for packer_fmt.sh so that it can interpret packer options.

I did some preliminary testing by hand with a test file and all the tests gave the expected results. I need your help in getting this well tested and getting the PR into a semblance of something that can be published.

Would you be able to help.

I have not worked with pre-commit hooks and don't really know how to test locally with the whole pipeline.

nandac commented 2 years ago

@jsf9k Further to my previous comment I have tested it on my private repo like this:

- repo: https://github.com/nandac/pre-commit-packer
    rev: f0b7c8b6a206b2fa2a802a0a71d75cfac49dc5b5
    hooks:
      - id: packer_fmt
        args: ["-write=true"]

And it works well. I will keep testing.

mcdonnnj commented 1 month ago

Please note that as of the 0.1.0 release you should be able to pass commands to both hooks, and the packer_fmt hook specifically has had its default behavior moved into default arguments for the hook so it is easily overridden now.