chore(make): drop broken recipes, pin dev tools, add ci to .PHONY#1245
chore(make): drop broken recipes, pin dev tools, add ci to .PHONY#1245cristim wants to merge 2 commits into
Conversation
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 44 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR consolidates dev tool installation into a centralized Makefile mechanism with pinned versions and removes two broken Makefile targets (cost-estimate and profile-new) that invoked non-existent scripts. Error messages now direct users to ChangesDev Tool Pinning and Broken Target Removal
🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terraform/profiles/README.md (1)
63-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix profile paths in the new copy-from-example commands.
The commands use
profiles/..., but this README is forterraform/profiles, so from repo root these examples should referenceterraform/profiles/...to avoid copy/paste failure.Suggested docs fix
-# Copy example profile -cp profiles/aws/dev.tfvars profiles/aws/my-profile.tfvars +# Copy example profile +cp terraform/profiles/aws/dev.tfvars terraform/profiles/aws/my-profile.tfvars # Edit with your settings -vim profiles/aws/my-profile.tfvars +vim terraform/profiles/aws/my-profile.tfvars # Use it terraform apply -var-file="../../../profiles/aws/my-profile.tfvars"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/profiles/README.md` around lines 63 - 71, Update the example shell commands in the README so the paths point to terraform/profiles instead of profiles (e.g., change occurrences of "profiles/aws/..." to "terraform/profiles/aws/...") so users copying the commands from repo root will find the correct files; update the three commands shown (cp, vim, terraform apply -var-file) to use the corrected path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@terraform/profiles/README.md`:
- Around line 63-71: Update the example shell commands in the README so the
paths point to terraform/profiles instead of profiles (e.g., change occurrences
of "profiles/aws/..." to "terraform/profiles/aws/...") so users copying the
commands from repo root will find the correct files; update the three commands
shown (cp, vim, terraform apply -var-file) to use the corrected path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1805f2f0-1545-4904-ba92-41eaefb5cc0b
📒 Files selected for processing (4)
MakefileMakefile.terraformdocs/DEVELOPMENT.mdterraform/profiles/README.md
💤 Files with no reviewable changes (1)
- docs/DEVELOPMENT.md
Fixes review findings HYG-05 and HYG-11: - Remove the cost-estimate (Makefile) and profile-new (Makefile.terraform) targets: they invoke scripts/cost-estimate.sh and scripts/generate-profile.sh, which never existed in repo history, so both targets fail immediately. Drop their help lines and the stale references in docs/DEVELOPMENT.md and terraform/profiles/README.md. - Add ci to .PHONY so a file named "ci" cannot mask the target. - Pin install-dev-tools to the CI versions instead of @latest: golangci-lint v2.10.1 (v2 module path), gosec v2.22.4, gocyclo v0.6.0, golang-migrate v4.19.1. staticcheck has no CI pin and is only used by scripts/security-scan.sh; pinned to v0.7.0. - Point "not installed" hints at make install-dev-tools instead of per-tool @latest go install commands. The docker-compose v1 part of HYG-11 was already fixed on main by 5f45f5c (ci: use docker compose v2 instead of docker-compose v1). Verified with make -n on every touched target in both Makefiles and go list -m on each pinned module version. Closes #1174, Closes #1181
cristim
force-pushed
the
fix/hyg-05-hyg-11-fix
branch
from
a71d26a to
367dee1
Compare
|
@coderabbitai review |
Rate Limit Exceeded
|
CodeRabbit flagged: cp/vim under "Copy from Example" used profiles/aws/... but the dir is terraform/profiles/aws/..., so users running the commands from repo root hit "file not found". Add the missing terraform/ prefix, annotate the intended cwd for each block (repo root for cp/vim; the env dir for terraform apply), and add the explicit `cd terraform/environments/aws/dev` step that the Quick Deployment section already uses, so the relative ../../../profiles/... path actually resolves.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adversarial review (PR #1245)Reviewed against the five risk surfaces from the brief; the Make and docs changes are sound, with one stale-doc finding from CR's earlier pass that I just addressed. Verified
Fixes pushed
Follow-up filed
UNSTABLE stateFailing checks are pre-existing on
None are caused by this PR. Not blocking. VerdictLGTM. CR re-ping posted ( |
|
Tip For best results, initiate chat on the files or code changes.
Agreed on the outcome: the Make/docs changes look sound, the README path fix addresses the stale-doc issue I raised, and the Good catch as well on (•ᴗ•)🐇 ✏️ Learnings added
|
1 participant
Problem
Review findings HYG-05 and HYG-11 (docs/reviews/codebase-review-2026-06-10.md):
make cost-estimate(Makefile) andmake -f Makefile.terraform profile-newinvokescripts/cost-estimate.shandscripts/generate-profile.sh, which never existed in repo history (git log --allover both paths returns nothing), so both advertised targets fail immediately with "No such file or directory"..PHONYomitsci, so a file namedciat the repo root silently masksmake ci.install-dev-toolsinstalled golangci-lint/gosec/staticcheck/gocyclo/migrate at@latest, contradicting the project's CI pinning practice and causing "passes locally, fails in CI" drift.Fix
cost-estimateandprofile-newtargets, their.PHONYentries and help lines, plus the stale references indocs/DEVELOPMENT.mdandterraform/profiles/README.md(which advertised the nonexistentgenerate-profile.sh, including a fictional source listing).cito.PHONY.install-dev-toolsto the CI versions via overridable Make variables: golangci-lint v2.10.1 (matchingci.yml, switched to the/v2module path required by v2), gosec v2.22.4 (pre-commit.yml), gocyclo v0.6.0 (ci.yml/pre-commit.yml), golang-migrate v4.19.1 (ci.yml/database-migration.yml). staticcheck has no CI pin and is only consumed byscripts/security-scan.sh; pinned to the latest stable v0.7.0.lint/complexity/complexity-report/security-scan-goatmake install-dev-toolsinstead of per-tool@latestgo install commands, so the pins cannot be bypassed by following the hint.Test evidence
make -npasses on every touched/adjacent target:ci,install-dev-tools,help,lint,complexity,complexity-report,security-scan-go,docker-compose-test,pre-commit, andMakefile.terraformhelp/profile-list/profile-show/deploy/plan.make -n cost-estimateandmake -f Makefile.terraform -n profile-newboth return "No rule to make target".cipresent,make -n cistill runs the recipe (pre-fix it would report "up to date").go list -mconfirms golangci-lint/v2 v2.10.1, gosec/v2 v2.22.4, gocyclo v0.6.0, migrate/v4 v4.19.1, honnef.co/go/tools v0.7.0.@latestin the Makefile; no remaining repo references tocost-estimate/profile-new/generate-profile.shoutside docs/reviews.Closes #1174, Closes #1181
Summary by CodeRabbit
Chores
cost-estimateandprofile-newmake targets.Documentation