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

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

Open
nandac opened this issue Apr 23, 2022 · 10 comments
Open

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

nandac opened this issue Apr 23, 2022 · 10 comments
Assignees
Labels
bug This issue or pull request addresses broken functionality

Comments

@nandac
Copy link

nandac commented Apr 23, 2022

🐛 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
  1. 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
Copy link
Member

jsf9k commented May 4, 2022

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 jsf9k added the bug This issue or pull request addresses broken functionality label May 4, 2022
@jsf9k
Copy link
Member

jsf9k commented May 4, 2022

@mcdonnnj, is there possibly a typo here?

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

Should that instead be this?

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

@nandac
Copy link
Author

nandac commented May 4, 2022

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
Copy link
Member

mcdonnnj commented May 4, 2022

@mcdonnnj, is there possibly a typo here?

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

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
Copy link
Member

mcdonnnj commented May 4, 2022

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
Copy link
Author

nandac commented May 4, 2022

@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
Copy link
Member

jsf9k commented May 4, 2022

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

@nandac
Copy link
Author

nandac commented May 4, 2022

@jsf9k I have added this PR #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
Copy link
Author

nandac commented May 5, 2022

@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 mcdonnnj mentioned this issue Sep 10, 2024
6 tasks
@mcdonnnj
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality
Projects
None yet
Development

No branches or pull requests

3 participants