[dead-code] chore: remove dead functions — 1 function removed#39422
[dead-code] chore: remove dead functions — 1 function removed#39422github-actions[bot] wants to merge 1 commit into
Conversation
The function was only used by its own test (TestArcDindChrootConfigInjection). In production, the patch body is embedded directly inside BuildAWFCommand's arcDindPrefixProbe if-block via buildArcDindChrootConfigPatchBody(), so the standalone wrapper was never called from any main entrypoint. Remove the function, its exclusive test, and clean up dangling references in comments in awf_config.go and awf_helpers.go. Detected by: deadcode ./cmd/... ./internal/tools/... Run ID: 27559409182 Co-authored-by: Copilot <[email protected]>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39422 does not have the implementation label and has only 1 new line of code in business logic directories (well below the 100-line threshold). Neither enforcement condition is met. |
There was a problem hiding this comment.
Pull request overview
Removes a now-unused standalone ARC/DinD chroot config injection helper and its dedicated execution-based test, while keeping the inlined runtime patching behavior used by BuildAWFCommand.
Changes:
- Removed
buildArcDindChrootConfigInjectScriptfrompkg/workflow/awf_helpers.go. - Removed
TestArcDindChrootConfigInjection(and now-unused imports) frompkg/workflow/awf_helpers_test.go. - Updated
AWFConfigFile.Chrootfield comment to reflect runtime injection without referencing the removed helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/awf_helpers.go | Deletes the standalone chroot injection helper; retains embedded patch body used in BuildAWFCommand. |
| pkg/workflow/awf_helpers_test.go | Removes the Python-execution test and trims imports; leaves only string-based assertions for chroot injection presence. |
| pkg/workflow/awf_config.go | Updates the Chroot field comment to remove the removed-function reference. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| // buildArcDindChrootConfigPatchBody returns the Python heredoc that patches the AWF | ||
| // config file with chroot.binariesSourcePath and chroot.identity.*. It is designed to be | ||
| // embedded inside a bash if-block that already guards on DOCKER_HOST=tcp://... |
| // TestBuildAWFCommand_IncludesChrootInjectScript verifies that BuildAWFCommand | ||
| // includes the chroot injection script in the generated run step when the AWF | ||
| // version supports it. |
🧪 Test Quality Sentinel Report➖ Test Quality Score: N/A — No New Tests
📊 Metrics & Test Classification (0 tests analyzed)
Go: 0 ( Changed test files: Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out — clean dead-code removal with one noted coverage tradeoff, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Coverage reduction:
TestArcDindChrootConfigInjectionwas the only test that executed the Python chroot-injection script at runtime (6 DOCKER_HOST scenarios, JSON round-trip, dual-path writes). With its removal,buildArcDindChrootConfigPatchBody— still production code — has no runtime test coverage. This is an expected tradeoff for dead-code cleanup; flagged for team awareness.
Positive Highlights
- ✅ Correct removal:
buildArcDindChrootConfigInjectScripthad zero production callers; it existed solely as a test-convenience wrapper. - ✅ Comment in
awf_config.goupdated cleanly to remove stale function reference. - ✅
buildArcDindChrootConfigPatchBodycomment also cleaned up with no stale cross-reference. - ✅ Verified by
go build,go vet,go vet -tags=integration, andmake fmt.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 127.1 AIC · ⌖ 14.7 AIC · ⊞ 27.9K
| } | ||
| } | ||
|
|
||
| // TestBuildAWFCommand_IncludesChrootInjectScript verifies that BuildAWFCommand |
There was a problem hiding this comment.
[/zoom-out] This function is now the sole test covering the ARC/DinD chroot injection path — but it verifies that the script is included in the generated command, not that the Python executes correctly at runtime.
💡 Coverage tradeoff worth noting
TestArcDindChrootConfigInjection (removed) was the only test that:
- Actually ran the bash+Python script end-to-end
- Exercised 6 DOCKER_HOST scenarios (
(redacted)(redacted) empty, bare path...) - Verified JSON round-trip correctness (original fields preserved after patching)
- Confirmed writes to both
$RUNNER_TEMP/gh-aw/awf-config.jsonandawfArcDindChrootBinariesSourcePath
With its removal, the correctness of the Python in buildArcDindChrootConfigPatchBody (still production code) is no longer exercised by any unit or integration test. This is a deliberate tradeoff of the dead-code cleanup — just flagging it so the team knows the gap exists if the Python ever needs to change.
One lightweight option: a table-driven test on buildArcDindChrootConfigPatchBody that invokes python3 -c with a few controlled inputs could restore the most critical coverage without needing the removed wrapper.
There was a problem hiding this comment.
The dead code removal is mechanically correct — buildArcDindChrootConfigInjectScript has zero callers outside its own test, import cleanup is consistent, and comment updates are accurate.
One non-blocking coverage gap is worth tracking before the Python script is modified.
💡 Coverage regression: buildArcDindChrootConfigPatchBody() Python script is no longer executed by any test
TestArcDindChrootConfigInjection was the only test that actually ran the Python heredoc produced by buildArcDindChrootConfigPatchBody(). It covered:
- The
DOCKER_HOSTregex guard (positive: (redacted) negative: (redacted) empty) - The JSON read-modify-write round-trip (all pre-existing config keys preserved)
- The dual-write to both
$RUNNER_TEMP/gh-aw/awf-config.jsonandawfArcDindChrootBinariesSourcePath/awf-config.json identity.uid/gidpopulation viaid -u/id -gsubprocesses
TestBuildAWFCommand_IncludesChrootInjectScript (still present) only does assert.Contains string checks on the rendered command text — it never invokes bash or Python. A Python syntax error, a wrong heredoc quoting, or a broken id subprocess call would ship silently.
Suggested follow-up (not blocking): add a direct execution test for buildArcDindChrootConfigPatchBody() similar to the removed test but targeting the body function directly, or restore the meaningful assertions from the removed test inside TestBuildAWFCommand_IncludesChrootInjectScript.
🔎 Code quality review by PR Code Quality Reviewer · 174.2 AIC · ⌖ 13.1 AIC · ⊞ 17.3K
|
Recheck mergeability after GitHub refreshes the PR.
|
|
@zarenner just fyi |
Functions Removed
buildArcDindChrootConfigInjectScriptpkg/workflow/awf_helpers.goTests Removed
TestArcDindChrootConfigInjection(pkg/workflow/awf_helpers_test.go)Verification
go build ./...go vet ./...go vet -tags=integration ./...make fmthttps://github.com/github/gh-aw/actions/runs/27559409182