Repository navigation
chore(packages): drop duplicate test targets - #3352
Conversation
The package Makefile declared an identical `test` target twice, so every invocation printed "overriding commands for target 'test'" and "ignoring old commands". Make already ran the second recipe and discarded the first, so removing the first is behaviour-preserving. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/database
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request cleans up the build configuration for the MariaDB package by removing a duplicate 'test' target definition. This change resolves warnings generated during the build process where the Makefile was overriding commands, ensuring a cleaner output without impacting the existing test functionality. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
the
size/XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the test target from the packages/apps/mariadb/Makefile, which previously executed helm unittest .. I have no feedback to provide on this change.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
📝 WalkthroughWalkthroughEarly or duplicate ChangesMakefile test target cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Three package Makefiles declared an identical `test` target twice, so every invocation printed "overriding commands for target 'test'" and "ignoring old commands". Make names the lines in that warning: it was already running the second recipe and discarding the first, so the first declaration was dead in each case and removing it changes nothing but the noise. Which copy is dead is not the same in all three, so the edit is not mechanical. In monitoring the `.PHONY: test` line sits above the dead recipe rather than the live one; `.PHONY` applies to the target name wherever it is declared, so the declaration stays and only the recipe under it goes. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
changed the title
github-actions
Bot
added
the
area/uncategorized
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — the dead copy was correctly identified in all three files, and make's own diagnostics prove it.
Business context: three package Makefiles declared test twice, printing "overriding commands for target" on every run; make executed the second copy and discarded the first.
Verified per file
Make names the lines itself. On origin/main:
kafka Makefile:12: overriding recipe for target 'test' / Makefile:5: ignoring old recipe
mariadb Makefile:19: overriding recipe for target 'test' / Makefile:7: ignoring old recipe
monitoring Makefile:17: overriding recipe for target 'test' / Makefile:10: ignoring old recipe
Last definition wins, so in all three the live copy is the later one and the dead copy is the earlier one — and that is what this PR deletes in each case:
packages/apps/kafka/Makefile— dead 4-5, live 11-12; deletes 4-6.packages/apps/mariadb/Makefile— dead 6-7, live 18-19; deletes 6-8.packages/extra/monitoring/Makefile— dead 9-10, live 16-17; deletes 9-10, and correctly retains the.PHONY: testat line 8.
I reconstructed each edit locally and confirmed the result is byte-identical to the PR's blobs.
The live recipe is unchanged. Before and after, all three run helm unittest . with exit 0; the only delta is that the override warnings are gone. As it happens all six recipes were identical, so deleting the live copy instead would also have been harmless — there was no way to get this wrong behaviourally, which is worth knowing but does not detract from the change.
No package drops out of test coverage. hack/helm-unit-tests.sh:22 gates inclusion on make -C "$dir" -n test exit code; measured after the change, all three still gate exit 0. Suites pass: kafka 35, monitoring 9, mariadb 15 on current main.
The default goal is unaffected — .DEFAULT_GOAL = help before and after in all three; removing the first target does not shift it.
Follow-up (non-blocking, wording only)
The PR body says "which copy is dead is not the same across the three". That is imprecise in a way that could mislead the next reader about make's semantics: the dead copy is the first in all three files. What actually differs between them is where .PHONY: test sits relative to the duplicate pair. Worth a small wording fix; no code change.
Note on the test-count difference
The body cites mariadb at 11 tests where main now shows 15. Not a discrepancy — it was accurate at the branch point, and main has since gained tests/startup_probe_test.yaml (+4) via #3344.
2 participants
What this PR does
Three package Makefiles —
apps/mariadb,apps/kafkaandextra/monitoring— declared an identicaltesttarget twice, so every invocation printed:Make names the lines in that warning itself: it was already running the second recipe and discarding the first, so the earlier declaration was dead in each case and removing it changes nothing but the noise. All six recipes were
helm unittest ..Which copy is dead is not the same across the three, so this is not a mechanical edit. In
monitoringthe.PHONY: testline sits above the dead recipe rather than the live one; since.PHONYapplies to the target name wherever it is declared, the declaration stays and only the recipe beneath it goes.Default goals are unaffected:
hack/package.mksets.DEFAULT_GOAL=helpand is included ahead of any target in these files, sotestwas never selected by first-target-wins. Confirmed withmake -p --dry-runbefore and after, and each package's tests still run — 11, 35 and 9 respectively, with no warnings left.Release note
Summary by CodeRabbit