feat(pre-commit-hook): Initial implementation in python #22 #524

dannyvassallo · 2024-06-26T20:47:11Z

Issue #, if available: 22

Description of changes:

Adds a python based cross platform pre-commit-hook with unit and integration tests for cfn-guard.

Will follow up with docs updates and a tag for the hook to update the pre-commit-config.yml used by tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

benbridts · 2024-07-03T09:31:39Z

I love this!

Would it be possible to pass files to the python wrapper, and have that dynamically construct the --data argument? That way only changed files would be scanned.

Usage would be

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - validate
          - --rules=./guard/resources/validate/rules-dir/

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/

benbridts · 2024-07-03T09:39:37Z

pre_commit_hooks/cfn_guard.py

+    global version conflicts with existing installations, rust,
+    and cargo.
+    """
+    latest_tag = get_latest_tag()


I don't think this should be (or long term: stay) the latest tag, but the same version as the ref (which I realise is not easy).

Reasons:
- If there is a major version release, rule files might stop working
- currently no way to lock on a release version
- That's how other pre-commit hooks work

One way to solve this might be reading the version from cargo.toml

I like this! I tried to keep the tags separate since we use the non-prefixed versions of tags for the release url, the action- prefixed versions of tags for the github action, and the precommit- prefixed versions of tags for the precommit hook but tying the version to the one in the cargo.toml seems reasonable.

benbridts · 2024-07-03T09:42:32Z

pre_commit_hooks/cfn_guard.py

+    "https://api.github.com/repos/aws-cloudformation/cloudformation-guard/releases/latest"
+)
+BIN_NAME = "cfn-guard"
+UNSUPPORTED_OS_MSG = "Unsupported operating system. Could not install cfn-guard."


Another thing to consider is to bind to the rust library (and publishing wheels to pypi for different OSes). That way you still get a download for supported OSes, but instead of failing on unsupported OSes, customers can install a python/rust toolchain and build from source code

EDIT: I am excited to see this, this might all be possible future improvements instead

That's an interesting approach. This implementation should cover win, mac, and linux so I dropped this error in to see what the customer demand is for other operating systems outside of those three. I like this idea though.

dannyvassallo · 2024-07-03T13:24:06Z

I love this!

Would it be possible to pass files to the python wrapper, and have that dynamically construct the --data argument? That way only changed files would be scanned.

Usage would be

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - validate
          - --rules=./guard/resources/validate/rules-dir/

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/

I considered only using validate and setting up the arguments manually before passing them in. I didn't want to remove the ability to run tests pre-commit and the path of least resistance (mapping all of the args) was to just foward them along and allow customers to use the binary as they would outside of the framework. This is a nice take though. I'm not opposed.

benbridts · 2024-07-04T13:54:52Z

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving
# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/
I considered only using validate and setting up the arguments manually before passing them in. I didn't want to remove the ability to run tests pre-commit and the path of least resistance (mapping all of the args) was to just foward them along and allow customers to use the binary as they would outside of the framework. This is a nice take though. I'm not opposed.

I like the way https://github.com/python-jsonschema/check-jsonschema/blob/main/.pre-commit-hooks.yaml handles that by defining multiple hooks. For cfn-guard the usage could look like:

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard-validate # could also be just cfn-guard, assuming most people want this
        args:
          - --rules=./guard/resources/validate/rules-dir/
      - id: cfn-guard-base  # could als be just cfn-guard
        args:
          - test  # or validate
          - --test-data ...
          - --rules=./guard/resources/validate/rules-dir/

If it all possible: an option(or options) that accepts filenames as input, makes pre-commit a lot easier to use

…ore tests aws-cloudformation#22

dannyvassallo · 2024-07-09T15:03:18Z

@benbridts After some tinkering I settled on this:

-   repo: hook.url
    rev: pre-commit-vX.X.X
    hooks:
        -   id: cfn-guard
            args:
                - "--operation=validate"
                - "--rules=guard/resources/validate/rules-dir/"
            files: ^guard/resources/validate/data-dir/.*
        -   id: cfn-guard
            args:
                - "--operation=test"
                - "--dir=pre_commit_hooks_tests/resources/"
            files: ^pre_commit_hooks_tests/resources.*

This leverages the filenames arg from pre-commit and allows the user to specify the pattern for the directories/files that change through the framework rather than guard itself.

I also hardcoded the version number in the pre-commit hook so it's no longer pulling in the latest and the pre-commit rev will be tied to a specific version of guard starting with 3.1.1

…ation#528) * fix(fuzzer): Update Errors to use formatted vec string

…ation#22 (aws-cloudformation#524) * feat(pre-commit-hook): Initial implementation in python * feat(pre-commit-hook): Use filenames from hook, update api, and add more tests aws-cloudformation#22

commit a81cfd2 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:43:07 2024 -0400 remove logging commit 45dfeed Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:40:38 2024 -0400 parse commit bf83583 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:36:58 2024 -0400 check os commit fc503d9 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:27:58 2024 -0400 more logging commit 53bdc22 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:14:48 2024 -0400 logging commit c309a19 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:11:37 2024 -0400 rollback again commit cf23601 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:08:51 2024 -0400 do nothing commit 47c50d7 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 12:01:07 2024 -0400 update and bundle commit 9eea059 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:54:50 2024 -0400 rollback commit de8928f Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:52:45 2024 -0400 rules commit 47411b6 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:48:40 2024 -0400 Update index.js commit fa8e7b7 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:45:37 2024 -0400 Update index.ts commit a4c06b1 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:43:20 2024 -0400 try escaping file paths commit 45d4cf9 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Wed Jul 10 11:31:47 2024 -0400 new bundle commit 2ab135b Author: Dan Vassallo <dannyvassallo@users.noreply.github.com> Date: Tue Jul 9 16:28:08 2024 -0400 fix(github-action): Handle nested repos after checkout on PR (aws-cloudformation#526) * fix(github-action): Handle nested reposafter checkout on PR commit db8e206 Author: Dan Vassallo <dannyvassallo@users.noreply.github.com> Date: Tue Jul 9 16:21:19 2024 -0400 feat(pre-commit-hook): Update documentation and tag (aws-cloudformation#530) * feat(pre-commit-hook): Update documentation and tag commit 8b1606c Author: Dan Vassallo <dannyvassallo@users.noreply.github.com> Date: Tue Jul 9 13:58:48 2024 -0400 feat(pre-commit-hook): Initial implementation in python aws-cloudformation#22 (aws-cloudformation#524) * feat(pre-commit-hook): Initial implementation in python * feat(pre-commit-hook): Use filenames from hook, update api, and add more tests aws-cloudformation#22 commit 3b5d369 Author: Dan Vassallo <dannyvassallo@users.noreply.github.com> Date: Tue Jul 9 13:29:33 2024 -0400 fix(fuzzer): Update Errors to use formatted vec string (aws-cloudformation#528) * fix(fuzzer): Update Errors to use formatted vec string commit 9040ac2 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 16:03:45 2024 -0400 try different replacement commit e446ab4 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 15:45:33 2024 -0400 update guard lib commit 697a222 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 15:37:52 2024 -0400 Update typescript_library.yml commit 7dd62ea Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 15:37:01 2024 -0400 bundle commit 7e57559 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 15:29:41 2024 -0400 Update typescript_library.yml commit be8ad39 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 14:07:20 2024 -0400 catch validation error commit d730eab Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 12:06:28 2024 -0400 package bundle commit 8dd2d90 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 12:00:46 2024 -0400 Update utils.ts commit e56ed47 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 11:35:36 2024 -0400 Format readme for linter commit 5173867 Author: Dan Vassallo <danielvassallo87@gmail.com> Date: Tue Jul 2 11:21:39 2024 -0400 fix(github-action): Handle nested reposafter checkout on PR

…ation#22 (aws-cloudformation#524) * feat(pre-commit-hook): Initial implementation in python * feat(pre-commit-hook): Use filenames from hook, update api, and add more tests aws-cloudformation#22

feat(pre-commit-hook): Initial implementation in python

4b60889

dannyvassallo requested a review from joshfried-aws June 26, 2024 20:47

joshfried-aws approved these changes Jul 2, 2024

View reviewed changes

minhphuong98 approved these changes Jul 2, 2024

View reviewed changes

benbridts reviewed Jul 3, 2024

View reviewed changes

feat(pre-commit-hook): Use filenames from hook, update api, and add m…

5d23c99

…ore tests aws-cloudformation#22

dannyvassallo and others added 4 commits July 9, 2024 11:05

fix(documentation): Handle type

fbbbcf2

remove required libs

a973266

Update .pre-commit-config.yaml

32df63b

fix(fuzzer): Update Errors to use formatted vec string (aws-cloudform…

9f818bc

…ation#528) * fix(fuzzer): Update Errors to use formatted vec string

dannyvassallo merged commit 57d712c into aws-cloudformation:main Jul 9, 2024
25 checks passed

dannyvassallo deleted the precommit-dev branch July 9, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pre-commit-hook): Initial implementation in python #22 #524

feat(pre-commit-hook): Initial implementation in python #22 #524

dannyvassallo commented Jun 26, 2024 •

edited

Loading

benbridts commented Jul 3, 2024

benbridts Jul 3, 2024

dannyvassallo Jul 3, 2024 •

edited

Loading

benbridts Jul 3, 2024 •

edited

Loading

dannyvassallo Jul 3, 2024

dannyvassallo commented Jul 3, 2024

benbridts commented Jul 4, 2024 •

edited

Loading

dannyvassallo commented Jul 9, 2024

feat(pre-commit-hook): Initial implementation in python #22 #524

feat(pre-commit-hook): Initial implementation in python #22 #524

Conversation

dannyvassallo commented Jun 26, 2024 • edited Loading

benbridts commented Jul 3, 2024

benbridts Jul 3, 2024

Choose a reason for hiding this comment

dannyvassallo Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

benbridts Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

dannyvassallo Jul 3, 2024

Choose a reason for hiding this comment

dannyvassallo commented Jul 3, 2024

benbridts commented Jul 4, 2024 • edited Loading

dannyvassallo commented Jul 9, 2024

dannyvassallo commented Jun 26, 2024 •

edited

Loading

dannyvassallo Jul 3, 2024 •

edited

Loading

benbridts Jul 3, 2024 •

edited

Loading

benbridts commented Jul 4, 2024 •

edited

Loading