Conversation
TaichKarna
requested review from
arkid15r,
kasya and
rudransh-shrivastava
as code owners
github-actions
Bot
added
docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
Summary by CodeRabbit
WalkthroughAdds terraform-docs integration: CI installs terraform-docs, pre-commit runs a terraform-docs hook, a centralized config and Makefile target were added, and generated TF documentation blocks were injected into infrastructure module README files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
infrastructure/.terraform-docs.yaml (1)
1-37: Config looks good; one minor nit.The
recursive.enabled: falsesetting is correct here because the Makefile iterates modules explicitly viafind. A tiny readability nit: a blank line between theoutput:block andoutput-values:(after line 15) would match the spacing used elsewhere in the file.Also note: for this config to actually take effect in pre-commit/CI, the
terraform_docshook must pass--config=…/infrastructure/.terraform-docs.yaml— see the separate comment on.pre-commit-config.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/.terraform-docs.yaml` around lines 1 - 37, Add a blank line between the existing output: block and the output-values: block to match the file's spacing conventions; locate the output: and output-values: keys in .terraform-docs.yaml (and note recursive.enabled remains false) and insert a single empty line after the output: block to improve readability.infrastructure/README.md (1)
393-399: Minor:cd infrastructureis repo-root-relative, but earlier sectionscdinto subdirectories.A reader who followed the staging steps may already be inside
infrastructure/live/and will hit "No such file" oncd infrastructure. Consider clarifying: "From the repo root, runmake -C infrastructure docs-infrastructure" — this also avoids requiring a directory change at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/README.md` around lines 393 - 399, Update the Documentation section to clarify the command is repo-root-relative and avoid changing directories: replace the existing "cd infrastructure && make docs-infrastructure" guidance with a note like "From the repo root, run `make -C infrastructure docs-infrastructure`" (or explicitly state "If you are already inside infrastructure/live/, run `cd ../.. && make -C infrastructure docs-infrastructure`") so the make target `docs-infrastructure` is invoked without failing when the user is in a subdirectory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 67-77: Update the CI install step to clean up temporary files and
ensure version consistency: after the download/extract/install sequence that
uses TERRAFORM_DOCS_VERSION, add removal of the TARBALL and extracted
terraform-docs (rm -f "${TARBALL}" terraform-docs) to avoid leftover artifacts;
separately, document the pinned TERRAFORM_DOCS_VERSION in
infrastructure/README.md and add a version-check in the Makefile target that
runs terraform-docs (e.g., a check comparing terraform-docs --version against
TERRAFORM_DOCS_VERSION) so local devs and CI are validated against the same
pinned version.
In `@infrastructure/Makefile`:
- Around line 16-28: The docs-infrastructure Makefile target uses an unscoped
`find .` and an unchecked, whitespace-unsafe loop; change the `find` invocation
in the docs-infrastructure recipe to search only the infrastructure roots (e.g.
`infrastructure/modules` or explicitly `infrastructure/live
infrastructure/bootstrap infrastructure/state infrastructure/modules`) and
switch to NUL-separated handling (`-print0` on find and `while read -r -d ''
dir; do ... done`) ensuring you quote "$dir" when passing it to
`terraform-docs`, and after the `terraform-docs markdown "$dir" --config
$(CURDIR)/.terraform-docs.yaml --output-file README.md --output-mode inject`
call append `|| exit 1` so the loop (and make) fails immediately on any
terraform-docs error.
In `@infrastructure/modules/storage/README.md`:
- Around line 6-8: Update the provider version constraints from pessimistic ("~>
1.14.0", "~> 6.36.0", "~> 3.8.0") to exact pins ("1.14.0", "6.36.0", "3.8.0")
wherever the provider requirements are declared (look for the
requirement_terraform, requirement_aws, and requirement_random blocks in the
module's provider requirement declarations, including main.tf and any submodule
requirement blocks), then regenerate the module README so the table entries for
those anchors reflect the exact versions.
---
Nitpick comments:
In `@infrastructure/.terraform-docs.yaml`:
- Around line 1-37: Add a blank line between the existing output: block and the
output-values: block to match the file's spacing conventions; locate the output:
and output-values: keys in .terraform-docs.yaml (and note recursive.enabled
remains false) and insert a single empty line after the output: block to improve
readability.
In `@infrastructure/README.md`:
- Around line 393-399: Update the Documentation section to clarify the command
is repo-root-relative and avoid changing directories: replace the existing "cd
infrastructure && make docs-infrastructure" guidance with a note like "From the
repo root, run `make -C infrastructure docs-infrastructure`" (or explicitly
state "If you are already inside infrastructure/live/, run `cd ../.. && make -C
infrastructure docs-infrastructure`") so the make target `docs-infrastructure`
is invoked without failing when the user is in a subdirectory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 055e8c78-1bd2-4eef-8195-f1dc260c1bea
📒 Files selected for processing (24)
.github/workflows/run-ci-cd.yaml.markdownlint.yaml.pre-commit-config.yamlinfrastructure/.terraform-docs.yamlinfrastructure/Makefileinfrastructure/README.mdinfrastructure/bootstrap/README.mdinfrastructure/live/README.mdinfrastructure/modules/alb/README.mdinfrastructure/modules/cache/README.mdinfrastructure/modules/database/README.mdinfrastructure/modules/kms/README.mdinfrastructure/modules/networking/README.mdinfrastructure/modules/networking/modules/nacl/README.mdinfrastructure/modules/networking/modules/vpc-endpoint/README.mdinfrastructure/modules/parameters/README.mdinfrastructure/modules/security/README.mdinfrastructure/modules/service/README.mdinfrastructure/modules/storage/README.mdinfrastructure/modules/storage/modules/s3-bucket/README.mdinfrastructure/modules/storage/modules/shared-data-bucket/README.mdinfrastructure/modules/tasks/README.mdinfrastructure/modules/tasks/modules/task/README.mdinfrastructure/state/README.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 24 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: both findings are medium-severity (5/10) and focused on documentation automation rather than core runtime behavior.
- In
infrastructure/Makefile, failures fromterraform-docsinside the loop are not propagated, which can report success after partial doc generation and mask CI/doc drift. infrastructure/Makefilealso usesfind ., so invoking the Makefile from outsideinfrastructure/can scan unintended paths and produce inconsistent docs behavior.- Pay close attention to
infrastructure/Makefile- ensure loop errors fail the target and scopefindto the intended infrastructure directory.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/Makefile">
<violation number="1" location="infrastructure/Makefile:17">
P2: `find .` scopes the search relative to the working directory. If the Makefile is invoked from outside `infrastructure/` (e.g., `make -f infrastructure/Makefile docs-infrastructure`), `find .` will walk the entire repo and attempt to run `terraform-docs` on unrelated `main.tf` files. Scope the search to the known module roots (`bootstrap`, `live`, `state`, `modules`) as done by the sibling `test-infrastructure` target.</violation>
<violation number="2" location="infrastructure/Makefile:24">
P2: `terraform-docs` failures inside the loop are not propagated, so the Make target can succeed after only partial doc generation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/README.md">
<violation number="1" location="infrastructure/README.md:403">
P2: The Homebrew install command uses an invalid/nonexistent formula name, so the documented macOS setup path is broken.</violation>
</file>
<file name=".github/workflows/run-ci-cd.yaml">
<violation number="1" location=".github/workflows/run-ci-cd.yaml:77">
P2: Unchained cleanup command can mask terraform-docs download/verify/install failures and let the step succeed spuriously.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4562 +/- ##
=======================================
Coverage 98.93% 98.93%
=======================================
Files 527 527
Lines 16954 16954
Branches 2410 2358 -52
=======================================
Hits 16774 16774
Misses 97 97
Partials 83 83
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Hi @TaichKarna , thank you for working on this!
I have a question for you -- have you tried the official hooks instead of antonbabenko/pre-commit-terraform's hooks for terraform-docs?
I'm referring to terraform-docs/terraform-docs and the docker-based hook .
If so, what was your experience? Does it also require an additional step in CI to add the terraform-docs binary in $PATH?
|
Both approaches requires a script to find directories changed and running command against each of them something the current hook does behind the scenes but wont require extra setup steps. @rudransh-shrivastava |
I see, can you please update the code to use them when you get a chance? I want to avoid having to download a specific tarball in CI as it's hard to maintain. I don't know if Dependabot can update the version ( If Thank you @TaichKarna |
I tried the script approach but moving to using a script as hook means the download has to move somewhere else ( inside script, ci / cd or one more hook script for dowloading ). I found using go install in Ci/CD simpler and Ubuntu latest using in our job runner comes with Go preinstalled. I implemented and tested in a different branch, you can look at the approach here. https://github.com/TaichKarna/Nest/tree/terra-go @rudransh-shrivastava |
Please check this action run when you get a chance. It seems we don't need to download things manually -- the pre-commit hook manages that on it's own. There is no need to update the CI pipeline. The code for this is here. Why do we need a script? edit: the CI fail is intentional, it's just a PoC for pre-commit run. |
Terraform-docs natively only processes a single directory at a time. You point it at one path and it generates docs for that module, there's recursive flag but it doesnt work well with our structure. So we have to add command for all our directories or subdirectories seperately. A script would take changed tf files get their directories and run the terraform docs command against it, that what the antonbabenko hook does too. A custom script or the current hook both requires installation. Thank your for reviewing @rudransh-shrivastava |
I see what you mean, sub-modules inside modules aren't affected. This seems to be a limitation with |
|
I tried to fix it, let's see if this gets merged: terraform-docs/terraform-docs#924 |
This solves everything, good going!! Do let me know if you can when it gets merged. |
|
It has been merged -- let's wait for a release. 
|
2 participants
Proposed change
Resolves #4485
Terraform docs support for auto doc generation for infrastructure.
This PR:
.terraform-docs.ymlconfiguration in theinfrastructure/folderterraform_docshook into pre-commit to automatically regenerate module READMEs on commitMD060andMD034in.markdownlint.yamlto resolve conflicts with terraform-docs generated table and URL formattingdocs-infrastructuretarget in the Makefile to generate docs for all modules and nested submodules<!-- BEGIN_TF_DOCS -->/<!-- END_TF_DOCS -->injection markers across all modulesChecklist
make check-testlocally: all warnings addressed, tests passed