-
-
Notifications
You must be signed in to change notification settings - Fork 587
feat: Adding e2e hook tests #990
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
base: master
Are you sure you want to change the base?
Changes from all commits
2e5430b
73d8231
e1d2bce
b68074d
af727da
f9c96c4
2661af9
a6052ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| name: E2E hook tests | ||
|
|
||
| on: | ||
| merge_group: | ||
| pull_request: | ||
| push: | ||
| branches-ignore: | ||
| - dependabot/** # Dependabot always creates PRs | ||
| - renovate/** # Our Renovate setup always creates PRs | ||
| - gh-readonly-queue/** # Temporary merge queue-related GH-made branches | ||
| - pre-commit-ci-update-config # pre-commit.ci always creates a PR | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: >- | ||
| ${{ github.workflow }}-${{ github.ref_type }}-${{ | ||
| github.event.pull_request.number || github.sha }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| e2e: | ||
| name: Run e2e hook tests | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
|
|
||
| # The published image bundles every tool the hooks need (terraform, | ||
| # hcledit, pre-commit, git, ...). Hook and test code come from the checkout | ||
| # below, so the image only needs to provide tooling, not be in sync. | ||
| # Pinned by digest for reproducibility/supply-chain; bump via Renovate. | ||
| env: | ||
| # renovate: datasource=docker depName=ghcr.io/antonbabenko/pre-commit-terraform | ||
| # yamllint disable-line rule:line-length | ||
| IMAGE: ghcr.io/antonbabenko/pre-commit-terraform:latest@sha256:4ef4b8323b27fc263535ad88c9d2f20488fcb3b520258e5e7f0553ed5f6692b5 | ||
|
|
||
|
pen-pal marked this conversation as resolved.
|
||
| steps: | ||
| - name: Check out src from Git | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Pull the multi-tool image | ||
| run: docker pull "${IMAGE}" | ||
|
|
||
| # Checkout happens on the host (needs Node for the JS action); the tests | ||
| # run inside the image, which the alpine-based image can't host directly. | ||
| - name: Run e2e tests inside the image | ||
| run: >- | ||
| docker run --rm | ||
| --volume "${GITHUB_WORKSPACE}:/lint" | ||
| --workdir /lint | ||
| --entrypoint bash | ||
| "${IMAGE}" | ||
| tests/e2e/run_e2e_tests.sh | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a section about requesting the addition of e2e hooks to https://github.com/antonbabenko/pre-commit-terraform/blob/master/.github/CONTRIBUTING.md#add-new-hook |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,88 @@ | ||||||
| # End-to-end hook tests | ||||||
|
|
||||||
| Behavioral tests that run the hooks the same way a user would — through | ||||||
| `pre-commit` against real fixture files — and compare the result against a | ||||||
| committed "golden" output. See [issue #823][issue-823]. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never understand the idea of complicating things like this. It is used only in 1 place.
Suggested change
|
||||||
|
|
||||||
| ## Layout | ||||||
|
|
||||||
| ```text | ||||||
| tests/e2e/ | ||||||
| run_e2e_tests.sh # the runner | ||||||
| cases/<hook_id>/<case_name>/ | ||||||
| .pre-commit-config.yaml # repo:local config; __PCT_REPO__ -> repo root | ||||||
| input/ # working tree the hook runs against | ||||||
| expected/ # working tree as it should look AFTER the hook | ||||||
| expected_returncode # optional, default 0 (fixing hooks exit 1) | ||||||
| requires # optional, one CLI tool per line; SKIP if missing | ||||||
| ``` | ||||||
|
|
||||||
| A case passes when the `pre-commit run` exit code matches `expected_returncode` | ||||||
| **and** the resulting working tree is byte-identical to `expected/`. | ||||||
|
|
||||||
| Note: hooks that modify tracked files in place (e.g. `terraform_fmt`) make | ||||||
| `pre-commit` exit `1` ("files were modified by this hook"), so those cases set | ||||||
| `expected_returncode` to `1`. Hooks that only generate new files (e.g. | ||||||
| `terraform_wrapper_module_for_each`) leave tracked files untouched and exit `0`. | ||||||
|
|
||||||
| ## Prerequisites | ||||||
|
|
||||||
| The runner itself needs `pre-commit` and `git`. Each case additionally needs | ||||||
| the tools its hook calls; a case is **skipped** (not failed) when a tool listed | ||||||
| in its `requires` file is missing from `PATH`. | ||||||
|
|
||||||
| Current cases need: | ||||||
|
|
||||||
| | Tool | Used by | | ||||||
| | --- | --- | | ||||||
| | `pre-commit`, `git` | the runner (always) | | ||||||
| | `terraform` | `terraform_fmt` | | ||||||
| | `hcledit` | `terraform_wrapper_module_for_each` | | ||||||
|
|
||||||
| Install locally: | ||||||
|
|
||||||
| ```bash | ||||||
| # macOS (Homebrew) | ||||||
| brew install pre-commit terraform minamijoyo/hcledit/hcledit | ||||||
|
|
||||||
| # Linux: pre-commit via pip, terraform via HashiCorp's repo, and hcledit: | ||||||
| curl -L "$(curl -s https://api.github.com/repos/minamijoyo/hcledit/releases/latest \ | ||||||
| | grep -o -E -m 1 "https://.+?_linux_amd64.tar.gz")" > hcledit.tar.gz \ | ||||||
| && tar -xzf hcledit.tar.gz hcledit && rm hcledit.tar.gz && sudo mv hcledit /usr/bin/ | ||||||
| ``` | ||||||
|
|
||||||
| See the repo [README "How to install"](../../README.md#how-to-install) for the | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| full tool list and other install methods. | ||||||
|
|
||||||
| Or skip local installs entirely and use the project image, which bundles every | ||||||
| tool (this is what CI does) — see [Running](#running) below. | ||||||
|
|
||||||
| ## Running | ||||||
|
|
||||||
| Natively: | ||||||
|
|
||||||
| ```bash | ||||||
| bash tests/e2e/run_e2e_tests.sh | ||||||
| ``` | ||||||
|
|
||||||
| Inside the project image (matches CI — bundles every tool, no local installs): | ||||||
|
|
||||||
| ```bash | ||||||
| docker build -t pct:e2e --build-arg INSTALL_ALL=true . | ||||||
| docker run --rm -v "$PWD:/lint" -w /lint --entrypoint bash pct:e2e \ | ||||||
|
Comment on lines
+71
to
+72
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what's the reason to build a separate image? Just use a exisitng one https://github.com/antonbabenko/pre-commit-terraform#1-install-dependencies |
||||||
| tests/e2e/run_e2e_tests.sh | ||||||
| ``` | ||||||
|
|
||||||
| ## Adding a case | ||||||
|
|
||||||
| 1. Create `cases/<hook_id>/<case_name>/`. | ||||||
| 2. Add `input/` (the fixture) and a `.pre-commit-config.yaml` wiring the hook as | ||||||
| a `repo: local` hook with `entry: __PCT_REPO__/hooks/<hook>.sh`. | ||||||
| 3. Generate `expected/`: run the hook once against a copy of `input/` and commit | ||||||
| the resulting tree. Set `expected_returncode` if the hook modifies files. | ||||||
| 4. Add a `requires` file listing any non-trivial CLI tools the hook needs. | ||||||
|
|
||||||
| The runner auto-discovers any `cases/<hook_id>/<case_name>/` dir — no runner | ||||||
| changes are needed to cover a new hook. | ||||||
|
|
||||||
| [issue-823]: https://github.com/antonbabenko/pre-commit-terraform/issues/823 | ||||||
|
Comment on lines
+87
to
+88
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| repos: | ||
| - repo: local | ||
| hooks: | ||
| - id: terraform_fmt | ||
| name: terraform_fmt | ||
| language: script | ||
| entry: __PCT_REPO__/hooks/terraform_fmt.sh | ||
| files: \.(tf|tofu)$ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| variable "name" { | ||
| type = string | ||
| default = "demo" | ||
| } | ||
|
|
||
| output "id" { | ||
| value = var.name | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| variable "name" { | ||
| type=string | ||
| default = "demo" | ||
| } | ||
|
|
||
| output "id" { | ||
| value = var.name | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| terraform |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| repos: | ||
| - repo: local | ||
| hooks: | ||
| - id: terraform_wrapper_module_for_each | ||
| name: terraform_wrapper_module_for_each | ||
| language: script | ||
| entry: __PCT_REPO__/hooks/terraform_wrapper_module_for_each.sh | ||
| files: \.(tf|tofu)$ | ||
| args: | ||
| - --args=--module-repo-org=terraform-aws-modules | ||
| - --args=--module-repo-shortname=s3-bucket | ||
| - --args=--module-repo-provider=aws |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| variable "name" { | ||
| type = string | ||
| } | ||
|
|
||
| output "id" { | ||
| value = "x" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| terraform { | ||
| required_version = ">= 1.3" | ||
| required_providers { | ||
| aws = { | ||
| source = "hashicorp/aws" | ||
| version = ">= 6.27" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| # Wrapper for the root module | ||
|
|
||
| The configuration in this directory contains an implementation of a single module wrapper pattern, which allows managing several copies of a module in places where using the native Terraform 0.13+ `for_each` feature is not feasible (e.g., with Terragrunt). | ||
|
|
||
| You may want to use a single Terragrunt configuration file to manage multiple resources without duplicating `terragrunt.hcl` files for each copy of the same module. | ||
|
|
||
| This wrapper does not implement any extra functionality. | ||
|
|
||
| ## Usage with Terragrunt | ||
|
|
||
| `terragrunt.hcl`: | ||
|
|
||
| ```hcl | ||
| terraform { | ||
| source = "tfr:///terraform-aws-modules/s3-bucket/aws//wrappers" | ||
| # Alternative source: | ||
| # source = "git::git@github.com:terraform-aws-modules/terraform-aws-s3-bucket.git//wrappers?ref=master" | ||
| } | ||
|
|
||
| inputs = { | ||
| defaults = { # Default values | ||
| create = true | ||
| tags = { | ||
| Terraform = "true" | ||
| Environment = "dev" | ||
| } | ||
| } | ||
|
|
||
| items = { | ||
| my-item = { | ||
| # omitted... can be any argument supported by the module | ||
| } | ||
| my-second-item = { | ||
| # omitted... can be any argument supported by the module | ||
| } | ||
| # omitted... | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Usage with Terraform | ||
|
|
||
| ```hcl | ||
| module "wrapper" { | ||
| source = "terraform-aws-modules/s3-bucket/aws//wrappers" | ||
|
|
||
| defaults = { # Default values | ||
| create = true | ||
| tags = { | ||
| Terraform = "true" | ||
| Environment = "dev" | ||
| } | ||
| } | ||
|
|
||
| items = { | ||
| my-item = { | ||
| # omitted... can be any argument supported by the module | ||
| } | ||
| my-second-item = { | ||
| # omitted... can be any argument supported by the module | ||
| } | ||
| # omitted... | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Example: Manage multiple S3 buckets in one Terragrunt layer | ||
|
|
||
| `eu-west-1/s3-buckets/terragrunt.hcl`: | ||
|
|
||
| ```hcl | ||
| terraform { | ||
| source = "tfr:///terraform-aws-modules/s3-bucket/aws//wrappers" | ||
| # Alternative source: | ||
| # source = "git::git@github.com:terraform-aws-modules/terraform-aws-s3-bucket.git//wrappers?ref=master" | ||
| } | ||
|
|
||
| inputs = { | ||
| defaults = { | ||
| force_destroy = true | ||
|
|
||
| attach_elb_log_delivery_policy = true | ||
| attach_lb_log_delivery_policy = true | ||
| attach_deny_insecure_transport_policy = true | ||
| attach_require_latest_tls_policy = true | ||
| } | ||
|
|
||
| items = { | ||
| bucket1 = { | ||
| bucket = "my-random-bucket-1" | ||
| } | ||
| bucket2 = { | ||
| bucket = "my-random-bucket-2" | ||
| tags = { | ||
| Secure = "probably" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| module "wrapper" { | ||
| source = "../" | ||
|
|
||
| for_each = var.items | ||
|
|
||
| name = try(each.value.name, var.defaults.name) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| output "wrapper" { | ||
| description = "Map of outputs of a wrapper." | ||
| value = module.wrapper | ||
| # sensitive = false # No sensitive module output found | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| variable "defaults" { | ||
| description = "Map of default values which will be used for each item." | ||
| type = any | ||
| default = {} | ||
| } | ||
|
|
||
| variable "items" { | ||
| description = "Maps of items to create a wrapper from. Values are passed through to the module." | ||
| type = any | ||
| default = {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| terraform { | ||
| required_version = ">= 1.3" | ||
| required_providers { | ||
| aws = { | ||
| source = "hashicorp/aws" | ||
| version = ">= 6.27" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| variable "name" { | ||
| type = string | ||
| } | ||
|
|
||
| output "id" { | ||
| value = "x" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| terraform { | ||
| required_version = ">= 1.3" | ||
| required_providers { | ||
| aws = { | ||
| source = "hashicorp/aws" | ||
| version = ">= 6.27" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we pin it to hash, then there's no sense to use the latest at all, which contradicts the main idea - test againt latest tool set & against the latest stable hook version.
In other words, we need 2 tests: against
latestw/o pinning and against the latest available hook version, which can be bumped by renovate, but I'd like to have it as part of the release process.