refactor(make): move recipe logic into scripts/ and record the runner choice - #192
Conversation
… choice The task-runner choice was never written down -- it was inherited from the baseline and had no ADR, so "why make and not Taskfile?" had no answer to read. ADR-0019 records it, and argues the rejected alternative honestly: Taskfile is better than make on the axes it targets, and the reasons to stay are the extra required install in a repo whose prerequisites are already a documented hurdle, and a migration touching every workflow, doc, and exercise for no behavioural gain. The ADR names the trigger to reopen it. The second half of the problem was where logic lived. `test-backend` was 32 lines of shell inside a recipe -- backslash continuations, `$$` escaping, a readiness loop, an exit code captured and re-raised around a cleanup step -- and `clean` was 28. Recipe shell is the worst place for that: shellcheck cannot see it, it cannot be run in isolation, and it is written in a quoting compromise between shell and make. `test-backend`, `test-docker`, and `clean` now live in scripts/, invoked as one-line targets, and are covered by the existing shellcheck and shfmt hooks. COMPOSE stays defined once in the Makefile and is passed through the environment; each script fails loudly if it is missing rather than silently running against the wrong compose project. Makefile: 446 -> 373 lines. The line is control flow, not line count: `smoke` and `smoke-full` stay inline despite their length, because they are flat command sequences whose bulk is comments explaining the CI contract that .github/workflows/ points at as "runs exactly this". Behaviour verified in both directions, since a silent break here would mean green CI over failing tests: `make test-backend` passes 38 tests and exits 0, and with a deliberately failing test added it exits non-zero -- the exit code still survives the database-cleanup step. `make help` lists all 54 targets unchanged, and `make ci` still resolves. The extracted scripts also drop two non-ASCII characters the recipes carried, bringing them in line with the repo's ASCII-only convention.
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ 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 review |
✅ Action performedReview finished.
|
2 participants
The task-runner choice was never written down -- it was inherited from the baseline and had no ADR, so "why make and not Taskfile?" had no answer to read. ADR-0019 records it, and argues the rejected alternative honestly: Taskfile is better than make on the axes it targets, and the reasons to stay are the extra required install in a repo whose prerequisites are already a documented hurdle, and a migration touching every workflow, doc, and exercise for no behavioural gain. The ADR names the trigger to reopen it.
The second half of the problem was where logic lived.
test-backendwas 32 lines of shell inside a recipe -- backslash continuations,$$escaping, a readiness loop, an exit code captured and re-raised around a cleanup step -- andcleanwas 28. Recipe shell is the worst place for that: shellcheck cannot see it, it cannot be run in isolation, and it is written in a quoting compromise between shell and make.test-backend,test-docker, andcleannow live in scripts/, invoked as one-line targets, and are covered by the existing shellcheck and shfmt hooks. COMPOSE stays defined once in the Makefile and is passed through the environment; each script fails loudly if it is missing rather than silently running against the wrong compose project. Makefile: 446 -> 373 lines.The line is control flow, not line count:
smokeandsmoke-fullstay inline despite their length, because they are flat command sequences whose bulk is comments explaining the CI contract that .github/workflows/ points at as "runs exactly this".Behaviour verified in both directions, since a silent break here would mean green CI over failing tests:
make test-backendpasses 38 tests and exits 0, and with a deliberately failing test added it exits non-zero -- the exit code still survives the database-cleanup step.make helplists all 54 targets unchanged, andmake cistill resolves.The extracted scripts also drop two non-ASCII characters the recipes carried, bringing them in line with the repo's ASCII-only convention.