Merged
Conversation
…, and comprehensive tests - Fix healthcheck.go: use caller's ctx instead of context.Background(), fix "server" typo - Fix service.go formatting, benchmark test using assert in hot path - Add .dockerignore to prevent secrets leaking into Docker images - Rename local.env to local.env.example, gitignore local.env - Modernize Dockerfile: COPY instead of ADD, CGO_ENABLED=0, ca-certificates, EXPOSE ports - Fix Makefile: add GIT_BRANCH, fix bench -run pattern, add fmt/doc targets - Migrate to Go 1.24+ tool directive, remove tools/tools.go and templated go.sum - Update GitHub Actions to checkout@v4, setup-go@v5 with caching - Fix GitLab CI: remove go mod tidy conflict with -mod=readonly, use go tool cobertura - Fix 5 README typos (wrong make targets, misspellings) - Update Python test deps and tox to modern versions, remove stale .travis.yml - Add GitHub Actions CI for cookiecutter repo itself - Add comprehensive test suite (44 tests) covering structure, content, and parameterized configs - Add CLAUDE.md and .editorconfig to generated projects
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds GitHub Actions (unit and integration) and local Makefile-based test runners, removes Travis CI, updates Python tooling and tox matrix, introduces pytest cookiecutter bake fixtures and extensive generation tests, adds post-gen local.env setup, and modernizes template files (Docker, Makefile, CI, go.mod, editorconfig) plus small Go test/code tidy-ups. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant Python as Python Setup (3.13)
participant Cookiecutter as cookiecutter
participant Generated as Generated Project (myapp)
participant Make as Make (build/test/lint)
Runner->>Python: setup-python 3.13
Python->>Cookiecutter: pip install cookiecutter
Runner->>Cookiecutter: cookiecutter --no-input --verbose .
Cookiecutter->>Generated: render template -> workspace/myapp
Runner->>Generated: cd myapp
Runner->>Make: make build
Make->>Make: make test / make lint
Make-->>Runner: exit code / artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
ankurs
requested review from
fajran,
kevinjom,
rahulpr22,
svetha-cvl and
wdevarshi
There was a problem hiding this comment.
Pull request overview
Modernizes the Go microservice cookiecutter template to improve tooling/CI ergonomics, Docker security defaults, and add a much more comprehensive Python-based validation suite for template output.
Changes:
- Migrates tool dependency tracking from
tools/tools.goto the Gotooldirective ingo.mod, and updates Make/CI to usego tool. - Updates Dockerfile/Makefile/CI configs (Actions + GitLab CI), plus small Go fixes (healthcheck context, typo, formatting, benchmark).
- Adds/updates Python test infrastructure (new fixtures, expanded generation/content tests), modernizes tox/requirements, and removes Travis config.
Reviewed changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
{{cookiecutter.app_name}}/tools/tools.go |
Removes legacy “tools package” approach for pinning tool deps. |
{{cookiecutter.app_name}}/go.mod |
Adds tool directive entries for buf/lint/mockery/cobertura. |
{{cookiecutter.app_name}}/Makefile |
Switches to go tool usage; adds fmt/doc; updates build/bench behavior. |
{{cookiecutter.app_name}}/Dockerfile |
Uses WORKDIR + COPY, installs ca-certificates, exposes ports. |
{{cookiecutter.app_name}}/.dockerignore |
Reduces build context; excludes env files and misc artifacts. |
{{cookiecutter.app_name}}/.gitignore |
Ignores local.env, cover.html, and misc. |
{{cookiecutter.app_name}}/.github/workflows/go.yml |
Updates Actions versions and enables setup-go caching. |
{{cookiecutter.app_name}}/.gitlab-ci.yml |
Uses go tool for cobertura; adjusts GOFLAGS handling and removes vet job. |
{{cookiecutter.app_name}}/service/healthcheck.go |
Uses caller context and fixes readiness error message typo. |
{{cookiecutter.app_name}}/service/service.go |
Fixes receiver spacing/formatting. |
{{cookiecutter.app_name}}/service/service_test.go |
Makes benchmark timing/assertions more accurate for benchmarks. |
{{cookiecutter.app_name}}/README.md |
Fixes typos and updates Make target names/docs. |
{{cookiecutter.app_name}}/local.env.example |
Adds example local environment file. |
{{cookiecutter.app_name}}/CLAUDE.md |
Adds generated-project “agent instructions”/project notes. |
{{cookiecutter.app_name}}/.editorconfig |
Adds basic formatting defaults for common file types. |
hooks/post_gen_project.py |
Adjusts proto init steps and creates local.env from example. |
tests/conftest.py |
Adds fixtures to bake projects directly via cookiecutter without hooks. |
tests/test_cookiecutter_generation.py |
Replaces minimal tests with extensive structure/content/parameterized checks. |
requirements.txt |
Updates to modern, unpinned minimums for cookiecutter/pytest/tox/test deps. |
tox.ini |
Updates envlist to py311–py313 and removes unused deps. |
.github/workflows/test.yml |
Adds CI for running pytest across supported Python versions. |
.travis.yml |
Removes legacy Travis CI configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (9)
{{cookiecutter.app_name}}/.gitignore (1)
6-6: Consider anchoring themiscignore pattern.Line 6 ignores any path named
miscanywhere in the repo. If you only mean the repository-root folder, anchor it to reduce accidental ignores.Proposed tweak
-misc +/misc/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/.gitignore at line 6, The .gitignore entry "misc" currently matches any path segment named misc anywhere; change that pattern to an anchored root match (e.g., use "/misc" or "/misc/") so only the repository-root misc folder is ignored; update the entry in .gitignore (replace the unanchored "misc" line) to the anchored form to avoid accidentally ignoring similarly named directories elsewhere.tox.ini (1)
10-10: Usepytestinstead of the legacypy.testalias for consistency with current tox best practices.Line 10 should use
pytestdirectly, as current tox documentation and examples exclusively recommendpytestoverpy.testfor new configurations. Whilepy.testremains supported in pytest 8.x+, adoptingpytestaligns with modern tooling standards.Proposed change
-commands = py.test {posargs:tests} +commands = pytest {posargs:tests}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tox.ini` at line 10, Replace the legacy py.test alias in the tox.ini commands setting with the modern pytest invocation: locate the "commands = py.test {posargs:tests}" entry and change "py.test" to "pytest" so the line reads using pytest directly; ensure no other occurrences of "py.test" remain in the file to keep tox configuration consistent with current best practices..github/workflows/test.yml (1)
1-25: LGTM! Well-structured CI workflow with modern action versions.The workflow correctly tests across Python 3.11-3.13 with pip caching enabled. Using
actions/checkout@v4andactions/setup-python@v5aligns with the PR's goal to update CI to modern versions.Optional improvements for robustness:
- Add
fail-fast: falseto the strategy to see all matrix results even if one fails- Add a
timeout-minutesto prevent indefinite hangs♻️ Optional: Add fail-fast and timeout
strategy: + fail-fast: false matrix: python-version: ["3.11", "3.12", "3.13"] steps: + timeout-minutes: 10 - uses: actions/checkout@v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 1 - 25, Add robustness to the Test job by disabling matrix fail-fast and adding a timeout: set strategy.fail-fast to false under the existing strategy.matrix block (referencing the strategy and matrix keys) and add a timeout-minutes entry under the test job (referencing jobs.test) with a sensible value (e.g., 10-30) so long-running runs are automatically cancelled.hooks/post_gen_project.py (1)
76-87: Add a progress message for consistency with other functions.The
setup_local_env()function works correctly, but unlikeinit_proto()andinit_git(), it doesn't print any status message. Adding one would provide consistent feedback to users.♻️ Suggested improvement for user feedback
def setup_local_env(): """ Copies local.env.example to local.env for local development """ import shutil example = os.path.join(PROJECT_DIRECTORY, "local.env.example") local = os.path.join(PROJECT_DIRECTORY, "local.env") if os.path.exists(example) and not os.path.exists(local): shutil.copy2(example, local) + print("Created local.env from local.env.example")Also, consider moving
import shutilto the top of the file with other imports for consistency with Python conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/post_gen_project.py` around lines 76 - 87, Add a user-facing progress message inside setup_local_env() to match init_proto() and init_git: when the function starts (or after copying), call the same logger/print used elsewhere to indicate "Creating local.env from local.env.example" or similar, and ensure it runs only when the copy occurs; also move the import shutil from inside setup_local_env to the module-level imports at the top of the file for consistency with other imports and style. Use the function name setup_local_env and the PROJECT_DIRECTORY constant to locate the code.requirements.txt (1)
1-7: Consider adding upper version bounds or using a lockfile for reproducible test environments.The shift from exact pins to minimum version constraints (
>=) provides flexibility but reduces reproducibility. All specified minimum versions exist and are accessible (cookiecutter 2.6.0, pytest 8.0.0, tox 4.0.0, pytest-cookies 0.7.0, binaryornot 0.4.4). For a template/testing repository this approach is acceptable, but adding upper bounds (e.g.,cookiecutter>=2.6.0,<3.0) or using a lockfile would ensure consistent CI test results across runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements.txt` around lines 1 - 7, The requirements.txt currently uses only minimum version constraints (cookiecutter, pytest, tox, pytest-cookies, binaryornot) which reduces reproducibility; update the constraints to include sensible upper bounds (e.g., cookiecutter>=2.6.0,<3.0) for each listed package or add a separate lockfile (pip-tools/poetry/requirements-lock.txt) and commit it, ensuring CI/test jobs consume the lockfile to pin exact versions reproducibly.tests/test_cookiecutter_generation.py (2)
315-327: Fragile YAML parsing.The before_script detection logic assumes 4-space indentation (
" -"). This works for the controlled template output but could break if indentation changes. Consider using a YAML parser for robustness, though this is acceptable for template tests.🧹 Alternative using PyYAML (if already a dependency)
def test_gitlab_ci_no_go_mod_tidy_in_before_script(self, bake_project): import yaml project = bake_project() content = (project / ".gitlab-ci.yml").read_text() data = yaml.safe_load(content) before_script = data.get("before_script", []) for cmd in before_script: assert "go mod tidy" not in cmd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 315 - 327, The test test_gitlab_ci_no_go_mod_tidy_in_before_script is fragile because it detects before_script by hard-coded 4-space indentation; replace the ad-hoc string scan with YAML parsing of the .gitlab-ci.yml content (e.g., use yaml.safe_load) and then assert that none of the entries in the parsed before_script list contain "go mod tidy", referencing the test function name and the ".gitlab-ci.yml" target.
206-210: Consider usingnext(iter(...))for single element access.Per static analysis, using
next(iter(...))is more idiomatic and avoids creating an intermediate list when only the first element is needed.🧹 Apply Ruff suggestion
def test_custom_service_name(self, bake_project): project = bake_project({"service_name": "OrderService"}) - proto_file = list((project / "proto").glob("*.proto"))[0] + proto_file = next(iter((project / "proto").glob("*.proto"))) content = proto_file.read_text() assert "service OrderService {" in content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 206 - 210, In test_custom_service_name replace the intermediate list creation when grabbing the first .proto file: in the test_custom_service_name function in tests/test_cookiecutter_generation.py change the proto_file assignment from list((project / "proto").glob("*.proto"))[0] to using next(iter((project / "proto").glob("*.proto"))) so you avoid allocating a list and follow the idiomatic pattern for single-element access.tests/conftest.py (1)
2-2: Unused import.The
osmodule is imported but not used anywhere in this file.🧹 Remove unused import
# -*- coding: utf-8 -*- -import os from pathlib import Path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` at line 2, Remove the unused top-level import statement "import os" from the file (it's not referenced anywhere in the module); delete that line and run the linter/tests to confirm no breakage, and only re-add the import if you later introduce code that uses the os module.{{cookiecutter.app_name}}/Makefile (1)
83-84: Consider addinggomarkdocto thetooldirective.The
doctarget usesgo run ...@latest, which downloads the tool on every invocation and isn't reproducible. Other tools (buf, golangci-lint, mockery) are managed via thetooldirective ingo.mod. For consistency and reproducibility, consider adding gomarkdoc to the tool block.♻️ Add to go.mod and update Makefile
In
go.mod:tool ( github.com/boumenot/gocover-cobertura github.com/bufbuild/buf/cmd/buf github.com/golangci/golangci-lint/v2/cmd/golangci-lint github.com/princjef/gomarkdoc/cmd/gomarkdoc github.com/vektra/mockery/v2 )Then in Makefile:
doc: - go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > README.md + go tool gomarkdoc ./... > README.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/Makefile around lines 83 - 84, The Makefile doc target uses "go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > README.md", which downloads the tool on every run; add "github.com/princjef/gomarkdoc/cmd/gomarkdoc" to the go.mod tool block (the tool (...) section) and update the Makefile doc target to call the tool without `@latest` (e.g., "go run github.com/princjef/gomarkdoc/cmd/gomarkdoc ./... > README.md") so gomarkdoc is versioned via the tool directive and invocations are reproducible, touching the go.mod tool list and the doc target in the Makefile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 1-25: Add robustness to the Test job by disabling matrix fail-fast
and adding a timeout: set strategy.fail-fast to false under the existing
strategy.matrix block (referencing the strategy and matrix keys) and add a
timeout-minutes entry under the test job (referencing jobs.test) with a sensible
value (e.g., 10-30) so long-running runs are automatically cancelled.
In @{{cookiecutter.app_name}}/.gitignore:
- Line 6: The .gitignore entry "misc" currently matches any path segment named
misc anywhere; change that pattern to an anchored root match (e.g., use "/misc"
or "/misc/") so only the repository-root misc folder is ignored; update the
entry in .gitignore (replace the unanchored "misc" line) to the anchored form to
avoid accidentally ignoring similarly named directories elsewhere.
In @{{cookiecutter.app_name}}/Makefile:
- Around line 83-84: The Makefile doc target uses "go run
github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > README.md", which
downloads the tool on every run; add
"github.com/princjef/gomarkdoc/cmd/gomarkdoc" to the go.mod tool block (the tool
(...) section) and update the Makefile doc target to call the tool without
`@latest` (e.g., "go run github.com/princjef/gomarkdoc/cmd/gomarkdoc ./... >
README.md") so gomarkdoc is versioned via the tool directive and invocations are
reproducible, touching the go.mod tool list and the doc target in the Makefile.
In `@hooks/post_gen_project.py`:
- Around line 76-87: Add a user-facing progress message inside setup_local_env()
to match init_proto() and init_git: when the function starts (or after copying),
call the same logger/print used elsewhere to indicate "Creating local.env from
local.env.example" or similar, and ensure it runs only when the copy occurs;
also move the import shutil from inside setup_local_env to the module-level
imports at the top of the file for consistency with other imports and style. Use
the function name setup_local_env and the PROJECT_DIRECTORY constant to locate
the code.
In `@requirements.txt`:
- Around line 1-7: The requirements.txt currently uses only minimum version
constraints (cookiecutter, pytest, tox, pytest-cookies, binaryornot) which
reduces reproducibility; update the constraints to include sensible upper bounds
(e.g., cookiecutter>=2.6.0,<3.0) for each listed package or add a separate
lockfile (pip-tools/poetry/requirements-lock.txt) and commit it, ensuring
CI/test jobs consume the lockfile to pin exact versions reproducibly.
In `@tests/conftest.py`:
- Line 2: Remove the unused top-level import statement "import os" from the file
(it's not referenced anywhere in the module); delete that line and run the
linter/tests to confirm no breakage, and only re-add the import if you later
introduce code that uses the os module.
In `@tests/test_cookiecutter_generation.py`:
- Around line 315-327: The test test_gitlab_ci_no_go_mod_tidy_in_before_script
is fragile because it detects before_script by hard-coded 4-space indentation;
replace the ad-hoc string scan with YAML parsing of the .gitlab-ci.yml content
(e.g., use yaml.safe_load) and then assert that none of the entries in the
parsed before_script list contain "go mod tidy", referencing the test function
name and the ".gitlab-ci.yml" target.
- Around line 206-210: In test_custom_service_name replace the intermediate list
creation when grabbing the first .proto file: in the test_custom_service_name
function in tests/test_cookiecutter_generation.py change the proto_file
assignment from list((project / "proto").glob("*.proto"))[0] to using
next(iter((project / "proto").glob("*.proto"))) so you avoid allocating a list
and follow the idiomatic pattern for single-element access.
In `@tox.ini`:
- Line 10: Replace the legacy py.test alias in the tox.ini commands setting with
the modern pytest invocation: locate the "commands = py.test {posargs:tests}"
entry and change "py.test" to "pytest" so the line reads using pytest directly;
ensure no other occurrences of "py.test" remain in the file to keep tox
configuration consistent with current best practices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 247fee00-c0b4-424e-b467-6951269f5375
⛔ Files ignored due to path filters (1)
{{cookiecutter.app_name}}/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/test.yml.travis.ymlhooks/post_gen_project.pyrequirements.txttests/conftest.pytests/test_cookiecutter_generation.pytox.ini{{cookiecutter.app_name}}/.dockerignore{{cookiecutter.app_name}}/.editorconfig{{cookiecutter.app_name}}/.github/workflows/go.yml{{cookiecutter.app_name}}/.gitignore{{cookiecutter.app_name}}/.gitlab-ci.yml{{cookiecutter.app_name}}/CLAUDE.md{{cookiecutter.app_name}}/Dockerfile{{cookiecutter.app_name}}/Makefile{{cookiecutter.app_name}}/README.md{{cookiecutter.app_name}}/go.mod{{cookiecutter.app_name}}/local.env.example{{cookiecutter.app_name}}/service/healthcheck.go{{cookiecutter.app_name}}/service/service.go{{cookiecutter.app_name}}/service/service_test.go{{cookiecutter.app_name}}/tools/tools.go
💤 Files with no reviewable changes (2)
- .travis.yml
- {{cookiecutter.app_name}}/tools/tools.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_cookiecutter_generation.py (2)
47-58: Keep this app-name test tied to the shared default context.This test only cares about slug derivation, but the inline
full_contextduplicates almost every template input. Each new cookiecutter field now creates an unrelated maintenance touchpoint here.Reduce the second source of truth
- def test_custom_app_name_with_spaces(self, bake_project): + def test_custom_app_name_with_spaces(self, bake_project, default_context): # Don't set app_name — let the Jinja2 filter derive it from name - project = bake_project(full_context={ - "source_path": "github.com/testorg", - "name": "My Cool Service", - "grpc_package": "com.github.testorg", - "service_name": "TestSvc", - "project_short_description": "A test service.", - "docker_image": "alpine:latest", - "docker_build_image": "golang", - "docker_build_image_version": "1.26", - }) + context = default_context.copy() + context.pop("app_name", None) + context["name"] = "My Cool Service" + project = bake_project(full_context=context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 47 - 58, The test_custom_app_name_with_spaces test duplicates the entire cookiecutter context; instead, rely on the shared default context and only override the field under test. Update the bake_project call in test_custom_app_name_with_spaces to remove the full_context bulk and pass a minimal override (e.g., only "name": "My Cool Service") via bake_project(full_context={...}) so the test focuses on slug derivation and avoids duplicating unrelated template inputs; keep using the same bake_project fixture and assertions as before.
315-327: Don't makebefore_scriptdetection depend on four-space indentation.A valid YAML reformat can make this loop stop inspecting
before_scriptwhile still passing, because only-lines are treated as entries.More robust scan without assuming exact indentation
- lines = content.split("\n") - in_before_script = False - for line in lines: - if "before_script:" in line: - in_before_script = True - elif in_before_script and not line.startswith(" -") and line.strip(): - in_before_script = False - if in_before_script: - assert "go mod tidy" not in line + before_script_blocks = re.findall( + r"(?ms)^\s*before_script:\s*\n((?:^\s*-\s.*\n?)*)", + content, + ) + assert before_script_blocks + for block in before_script_blocks: + assert "go mod tidy" not in block🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 315 - 327, The test test_gitlab_ci_no_go_mod_tidy_in_before_script currently detects the before_script block by hardcoding four-space indentation via checks for " -", which breaks if YAML is reformatted; change the logic in that test (the in_before_script detection loop) to first detect the before_script line using a regex like r'^\s*before_script\s*:' and record its leading indentation, then treat subsequent lines as list entries if they start with that indentation followed by '-' (e.g., r'^\s{N}\s*-\s') or any line that is more-indented than the before_script key; stop the block when you encounter a non-empty line whose indentation is less than or equal to the before_script key or a new top-level key (r'^\s*\S.*:'); this makes the assertion that "go mod tidy" is not present in before_script robust to reformatting while still checking the same variable names (in_before_script, lines, content).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cookiecutter_generation.py`:
- Around line 9-10: The current PATTERN/RE_OBJ only matches the simple "{{
cookiecutter.* }}" form; update the regex used by PATTERN (and thus RE_OBJ) so
test_no_unrendered_variables scans for other Jinja/Cookiecutter forms too—match
variable and block delimiters including {{ ... }}, {{- ... -}}, {% ... %}, {%-
... -%} (and optional whitespace control) when they contain "cookiecutter" or
other unrendered template tokens; change PATTERN accordingly and ensure
test_no_unrendered_variables uses RE_OBJ to detect any of these variants.
- Around line 249-253: The test test_dockerignore_excludes_secrets currently
only asserts presence of a few entries and doesn't prevent regression where
".git" could be added; update the test to read the .dockerignore via
bake_project(), split the content into lines (or strip and splitlines) and
assert that neither ".git" nor ".git/" appears as a whole line, while still
asserting the required entries (e.g., "local.env", "vendor", ".github") are
present as lines; reference the test function name
test_dockerignore_excludes_secrets and the bake_project fixture/project /
".dockerignore" read to locate where to change the assertions.
---
Nitpick comments:
In `@tests/test_cookiecutter_generation.py`:
- Around line 47-58: The test_custom_app_name_with_spaces test duplicates the
entire cookiecutter context; instead, rely on the shared default context and
only override the field under test. Update the bake_project call in
test_custom_app_name_with_spaces to remove the full_context bulk and pass a
minimal override (e.g., only "name": "My Cool Service") via
bake_project(full_context={...}) so the test focuses on slug derivation and
avoids duplicating unrelated template inputs; keep using the same bake_project
fixture and assertions as before.
- Around line 315-327: The test test_gitlab_ci_no_go_mod_tidy_in_before_script
currently detects the before_script block by hardcoding four-space indentation
via checks for " -", which breaks if YAML is reformatted; change the logic in
that test (the in_before_script detection loop) to first detect the
before_script line using a regex like r'^\s*before_script\s*:' and record its
leading indentation, then treat subsequent lines as list entries if they start
with that indentation followed by '-' (e.g., r'^\s{N}\s*-\s') or any line that
is more-indented than the before_script key; stop the block when you encounter a
non-empty line whose indentation is less than or equal to the before_script key
or a new top-level key (r'^\s*\S.*:'); this makes the assertion that "go mod
tidy" is not present in before_script robust to reformatting while still
checking the same variable names (in_before_script, lines, content).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6177c549-94c2-4566-ae7a-fb6231ce46e5
📒 Files selected for processing (4)
cookiecutter.jsontests/conftest.pytests/test_cookiecutter_generation.py{{cookiecutter.app_name}}/.dockerignore
✅ Files skipped from review due to trivial changes (2)
- {{cookiecutter.app_name}}/.dockerignore
- cookiecutter.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
- Fix GitLab CI cache: set GOMODCACHE to match cached path - Complete .PHONY list with all Makefile targets - Remove unused imports (os in conftest, Path in tests) - Add regression test: assert .git is NOT in .dockerignore
There was a problem hiding this comment.
🧹 Nitpick comments (3)
{{cookiecutter.app_name}}/Makefile (1)
83-84: Consider writing to a different file or appending.The
doctarget overwritesREADME.mdentirely with gomarkdoc output. If the README contains additional content (badges, setup instructions, examples), it will be lost. Consider writing to a dedicated file likeAPI.mdor documenting this behavior.♻️ Alternative: Write to a separate file
doc: - go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > README.md + go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > API.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/Makefile around lines 83 - 84, The Makefile's doc target currently overwrites README.md via the command "go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./... > README.md"; change it to write to a separate file (e.g. API.md) or append instead of overwrite (use >>) so existing README content (badges, examples) is preserved; update the doc target invocation string accordingly and add a brief comment in the Makefile explaining the chosen behavior.tests/test_cookiecutter_generation.py (2)
205-209: Usenext(iter(...))for single element access.Using list indexing on a generator is less efficient than using
next().♻️ Proposed fix
def test_custom_service_name(self, bake_project): project = bake_project({"service_name": "OrderService"}) - proto_file = list((project / "proto").glob("*.proto"))[0] + proto_file = next(iter((project / "proto").glob("*.proto"))) content = proto_file.read_text() assert "service OrderService {" in content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 205 - 209, Replace the inefficient list indexing used to get the single .proto file in test_custom_service_name: instead of building a list with list((project / "proto").glob("*.proto"))[0], use next(iter((project / "proto").glob("*.proto"))) to retrieve the first (and only) element; update the proto_file assignment in the test_custom_service_name function that uses bake_project so it uses next(iter(...)) for more efficient single-element access.
318-330: Hardcoded indentation assumption may be fragile.The logic assumes
before_scriptitems are indented with exactly 4 spaces (" -"). While this works for the controlled template, it could break if formatting changes. Consider using a YAML parser for robustness, or document the assumption.♻️ Alternative using YAML parsing
import yaml def test_gitlab_ci_no_go_mod_tidy_in_before_script(self, bake_project): project = bake_project() content = (project / ".gitlab-ci.yml").read_text() ci_config = yaml.safe_load(content) before_script = ci_config.get("before_script", []) for cmd in before_script: assert "go mod tidy" not in cmd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 318 - 330, The test test_gitlab_ci_no_go_mod_tidy_in_before_script is fragile because it assumes items in before_script are indented with exactly four spaces; change it to parse the .gitlab-ci.yml with a YAML parser (yaml.safe_load) and read the before_script key as a list, then iterate its elements and assert none contain "go mod tidy" to avoid brittle whitespace-based parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @{{cookiecutter.app_name}}/Makefile:
- Around line 83-84: The Makefile's doc target currently overwrites README.md
via the command "go run github.com/princjef/gomarkdoc/cmd/gomarkdoc@latest ./...
> README.md"; change it to write to a separate file (e.g. API.md) or append
instead of overwrite (use >>) so existing README content (badges, examples) is
preserved; update the doc target invocation string accordingly and add a brief
comment in the Makefile explaining the chosen behavior.
In `@tests/test_cookiecutter_generation.py`:
- Around line 205-209: Replace the inefficient list indexing used to get the
single .proto file in test_custom_service_name: instead of building a list with
list((project / "proto").glob("*.proto"))[0], use next(iter((project /
"proto").glob("*.proto"))) to retrieve the first (and only) element; update the
proto_file assignment in the test_custom_service_name function that uses
bake_project so it uses next(iter(...)) for more efficient single-element
access.
- Around line 318-330: The test test_gitlab_ci_no_go_mod_tidy_in_before_script
is fragile because it assumes items in before_script are indented with exactly
four spaces; change it to parse the .gitlab-ci.yml with a YAML parser
(yaml.safe_load) and read the before_script key as a list, then iterate its
elements and assert none contain "go mod tidy" to avoid brittle whitespace-based
parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5349169e-5fac-425f-87c4-8338e262fdca
📒 Files selected for processing (4)
tests/conftest.pytests/test_cookiecutter_generation.py{{cookiecutter.app_name}}/.gitlab-ci.yml{{cookiecutter.app_name}}/Makefile
✅ Files skipped from review due to trivial changes (1)
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- {{cookiecutter.app_name}}/.gitlab-ci.yml
Generated services are applications, not libraries. Users interact via gRPC/HTTP endpoints (documented by Swagger UI), not Go imports. The gomarkdoc-based doc target would overwrite the hand-written README.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
{{cookiecutter.app_name}}/Makefile (1)
1-1: Add anallphony target to satisfy Makefile tooling.
checkmakereports a missing required phony targetall. Add a simple alias totest(ordefault) to keep CI/static checks green.🔧 Proposed patch
-.PHONY: build build-alpine clean test default install generate run run-docker fmt help bench build-docker coverage-html dep lint mock runj +.PHONY: all build build-alpine clean test default install generate run run-docker fmt help bench build-docker coverage-html dep lint mock runj + +all: test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/Makefile at line 1, Add a phony "all" target to the Makefile so tooling like checkmake is satisfied: update the .PHONY line to include "all" and add a simple "all" target that aliases to the existing "test" or "default" target (e.g., define target "all:" that runs the same recipe as "test" or delegates to "default"), ensuring no duplicate recipes and keeping phony declarations consistent.tests/test_cookiecutter_generation.py (1)
205-209: Consider usingnext(iter(...))for single-element access.Per static analysis,
next(iter(...))is more idiomatic thanlist(...)[0]when you only need the first element. Alternatively, mirror the pattern fromtest_proto_file_existswhich explicitly asserts the count first.♻️ Suggested fix
def test_custom_service_name(self, bake_project): project = bake_project({"service_name": "OrderService"}) - proto_file = list((project / "proto").glob("*.proto"))[0] + proto_file = next(iter((project / "proto").glob("*.proto"))) content = proto_file.read_text() assert "service OrderService {" in content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 205 - 209, In test_custom_service_name, replace the non-idiomatic list((project / "proto").glob("*.proto"))[0] access with an idiomatic single-element access: use proto_file = next(iter((project / "proto").glob("*.proto"))) or alternatively follow the pattern from test_proto_file_exists by first asserting the number of matches and then indexing; update the test_custom_service_name (and use the bake_project/proto_file symbols) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @{{cookiecutter.app_name}}/Makefile:
- Line 1: Add a phony "all" target to the Makefile so tooling like checkmake is
satisfied: update the .PHONY line to include "all" and add a simple "all" target
that aliases to the existing "test" or "default" target (e.g., define target
"all:" that runs the same recipe as "test" or delegates to "default"), ensuring
no duplicate recipes and keeping phony declarations consistent.
In `@tests/test_cookiecutter_generation.py`:
- Around line 205-209: In test_custom_service_name, replace the non-idiomatic
list((project / "proto").glob("*.proto"))[0] access with an idiomatic
single-element access: use proto_file = next(iter((project /
"proto").glob("*.proto"))) or alternatively follow the pattern from
test_proto_file_exists by first asserting the number of matches and then
indexing; update the test_custom_service_name (and use the
bake_project/proto_file symbols) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49258e37-6d24-4086-b15e-9830e809514a
📒 Files selected for processing (2)
tests/test_cookiecutter_generation.py{{cookiecutter.app_name}}/Makefile
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
{{cookiecutter.app_name}}/Makefile:48
build-dockerpasses--build-arg GIT_BRANCH=..., but the Dockerfile only declaresARG GIT_COMMITandARG VERSION, so this build-arg is unused. Either addARG GIT_BRANCH(and use it, e.g., as a label) or remove the extra build-arg to keep Docker builds self-consistent.
build-docker:
@echo "building image ${BIN_NAME} ${VERSION} $(GIT_COMMIT)"
docker build --build-arg VERSION=${VERSION} --build-arg GIT_COMMIT=$(GIT_COMMIT) --build-arg GIT_BRANCH=$(GIT_BRANCH) -t $(IMAGE_NAME):local .
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ensures tool binaries (buf, golangci-lint, mockery) are installed into GOBIN in addition to being available via go tool.
- Add golang.org/x/vuln/cmd/govulncheck to go.mod tool block - Add make vulncheck target, run as prerequisite of make lint - Add vulnerability check step to GitHub Actions lint job - Bump go.opentelemetry.io/otel/* from v1.37.0 to v1.42.0 to fix GO-2026-4394 (arbitrary code execution via PATH hijacking)
Generates a full project with cookiecutter, then runs make build, test, and lint to verify the template produces a working service.
- scripts/test-e2e.sh: generates project in temp dir, runs build/test/lint - Makefile: make install (pip deps), make test (pytest), make test-e2e, make test-all - Add --verbose to cookiecutter in CI for better error visibility
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
{{cookiecutter.app_name}}/Makefile:50
build-dockerpasses--build-arg GIT_BRANCH=..., but the Dockerfile doesn't declare or useARG GIT_BRANCH(onlyGIT_COMMITandVERSION). This is currently a no-op and can be confusing; either remove the unused build arg or add a correspondingARG/LABELin the Dockerfile if you intend to surface branch metadata in the image labels.
build-docker:
@echo "building image ${BIN_NAME} ${VERSION} $(GIT_COMMIT)"
docker build --build-arg VERSION=${VERSION} --build-arg GIT_COMMIT=$(GIT_COMMIT) --build-arg GIT_BRANCH=$(GIT_BRANCH) -t $(IMAGE_NAME):local .
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace CLAUDE.md content with comprehensive AGENTS.md (architecture, workflows, rules); CLAUDE.md imports via @AGENTS.md - Fix git identity in CI workflow for post-gen hook git commit - Fix GitHub Actions concurrency groups to allow parallel jobs - Rename install target to deps (it only downloads modules)
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/test-e2e.sh (1)
17-18: Consider dynamically determining the project directory.The hardcoded
myapppath depends oncookiecutter.jsonhaving"name": "MyApp"which transforms tomyapp. If the default name changes, this script silently breaks. Consider finding the generated directory dynamically.More robust approach
cookiecutter --no-input --verbose --output-dir "$TMPDIR" "$REPO_DIR" -PROJECT_DIR="$TMPDIR/myapp" +# Find the generated project directory (should be the only subdirectory) +PROJECT_DIR=$(find "$TMPDIR" -mindepth 1 -maxdepth 1 -type d | head -1) +if [[ -z "$PROJECT_DIR" ]]; then + echo "ERROR: No project directory generated" + exit 1 +fi cd "$PROJECT_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-e2e.sh` around lines 17 - 18, The script currently hardcodes PROJECT_DIR="$TMPDIR/myapp" and then cd "$PROJECT_DIR", which will break if the generated project folder name differs; update the logic in scripts/test-e2e.sh to discover the created project directory dynamically (e.g., search $TMPDIR for the single non-temp directory or the newest/first directory created) and assign that path to PROJECT_DIR before the cd; reference the PROJECT_DIR variable and the cd "$PROJECT_DIR" call when locating where to change the assignment so the script works regardless of the cookiecutter "name" value.tests/test_cookiecutter_generation.py (1)
207-211: Prefernext(iter(...))over single-element slice.Using
next(iter(...))is more idiomatic and avoids constructing a full list just to access the first element.Suggested fix
def test_custom_service_name(self, bake_project): project = bake_project({"service_name": "OrderService"}) - proto_file = list((project / "proto").glob("*.proto"))[0] + proto_file = next(iter((project / "proto").glob("*.proto"))) content = proto_file.read_text() assert "service OrderService {" in content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cookiecutter_generation.py` around lines 207 - 211, The test creates proto_file by materializing a list then indexing [0], which unnecessarily builds a list; change the retrieval to use an iterator like next(iter((project / "proto").glob("*.proto"))) so test_custom_service_name uses next(iter(...)) for proto_file (keeping bake_project and the subsequent read_text/assert unchanged) to avoid allocating a full list when only the first element is needed.{{cookiecutter.app_name}}/AGENTS.md (1)
27-49: Add language hint to fenced code block.Static analysis flags the architecture diagram code block as missing a language. While this is an ASCII tree diagram, adding
textorplaintextsatisfies linters and improves rendering consistency.Suggested fix
-``` +```text . ├── main.go # Entry point: initializes ColdBrew, registers services🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/AGENTS.md around lines 27 - 49, Update the fenced ASCII tree in AGENTS.md to include a language hint (e.g., add "text" or "plaintext" after the opening triple-backticks) so the code block becomes a labeled plaintext block; this change targets the existing ASCII diagram block that begins with "." and the tree lines (e.g., "├── main.go") to satisfy linters and ensure consistent rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @{{cookiecutter.app_name}}/AGENTS.md:
- Around line 27-49: Update the fenced ASCII tree in AGENTS.md to include a
language hint (e.g., add "text" or "plaintext" after the opening
triple-backticks) so the code block becomes a labeled plaintext block; this
change targets the existing ASCII diagram block that begins with "." and the
tree lines (e.g., "├── main.go") to satisfy linters and ensure consistent
rendering.
In `@scripts/test-e2e.sh`:
- Around line 17-18: The script currently hardcodes PROJECT_DIR="$TMPDIR/myapp"
and then cd "$PROJECT_DIR", which will break if the generated project folder
name differs; update the logic in scripts/test-e2e.sh to discover the created
project directory dynamically (e.g., search $TMPDIR for the single non-temp
directory or the newest/first directory created) and assign that path to
PROJECT_DIR before the cd; reference the PROJECT_DIR variable and the cd
"$PROJECT_DIR" call when locating where to change the assignment so the script
works regardless of the cookiecutter "name" value.
In `@tests/test_cookiecutter_generation.py`:
- Around line 207-211: The test creates proto_file by materializing a list then
indexing [0], which unnecessarily builds a list; change the retrieval to use an
iterator like next(iter((project / "proto").glob("*.proto"))) so
test_custom_service_name uses next(iter(...)) for proto_file (keeping
bake_project and the subsequent read_text/assert unchanged) to avoid allocating
a full list when only the first element is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85edb16d-e58d-4f4d-93b4-e63ea759d72a
📒 Files selected for processing (9)
.github/workflows/integration.ymlMakefilescripts/test-e2e.shtests/test_cookiecutter_generation.py{{cookiecutter.app_name}}/.github/workflows/go.yml{{cookiecutter.app_name}}/AGENTS.md{{cookiecutter.app_name}}/CLAUDE.md{{cookiecutter.app_name}}/Makefile{{cookiecutter.app_name}}/go.mod
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/integration.yml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 53: Remove the leading shell prompt from the example command in
CONTRIBUTING.md: replace the line containing "$ git clone
git@github.com:your_name_here/cookiecutter-coldbrew.git" with the plain command
"git clone git@github.com:your_name_here/cookiecutter-coldbrew.git" so the
markdown no longer includes the "$" prompt (fixes markdownlint MD014).
- Add cookiecutter and pytest explicitly to tox deps, use pytest command - Move context.Background() and EchoRequest allocation outside benchmark loop
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 tasks
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive modernization of the cookiecutter-coldbrew template covering bug fixes, security improvements, CI upgrades, Go tooling modernization, and a full test suite.
Bug Fixes
healthcheck.go: use caller'sctxinstead ofcontext.Background(), fix "server" → "serve" typoservice.goformatting, benchmark test accuracy (removeassertfrom hot path)GOFLAGSconflict withgo mod tidy, setGOMODCACHEfor cache to workSecurity
.dockerignoreto prevent secrets leaking into Docker imageslocal.env, rename tolocal.env.examplegovulncheckfor dependency vulnerability scanning (make vulncheck, runs as part ofmake lint)go.opentelemetry.io/otel/*v1.37.0 → v1.42.0 (fixes GO-2026-4394)Docker & Build
COPYinstead ofADD,CGO_ENABLED=0,ca-certificates,EXPOSE 9090 9091, simplified layoutGIT_BRANCH, fix bench pattern, renameinstall→deps, addfmt/vulnchecktargets.PHONYlist, fix concurrency groups in GitHub ActionsGo Modernization
tools/tools.gowith Go 1.24+tooldirective ingo.modgo.sum(3,730 lines — regenerated by post-gen hook)CI/CD
.travis.yml, add GitHub Actions CI for cookiecutter repo itselfDeveloper Experience
AGENTS.md(vendor-neutral AI assistant instructions) with architecture, workflows, and rulesCLAUDE.mdimportsAGENTS.mdvia@AGENTS.md.editorconfigfor consistent formattingscripts/test-e2e.shand rootMakefile(make test,make test-e2e,make test-all)Tests
accept_hooks=False), completing in ~4sTest plan
pytest tests/ -v)make build && make test && make lint🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores