fix: broaden ARC/DinD detection to match any tcp:// DOCKER_HOST#31996
fix: broaden ARC/DinD detection to match any tcp:// DOCKER_HOST#31996salmanmkc wants to merge 6 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR broadens ARC/DinD topology detection so that any TCP-based DOCKER_HOST (tcp://...) triggers emission of AWF’s --docker-host-path-prefix argument, rather than only matching tcp://localhost / tcp://127.0.0.1.
Changes:
- Broadened the emitted bash
DOCKER_HOSTregex from localhost-only to^tcp://in AWF helper logic. - Updated the corresponding Copilot engine firewall args unit test snippet to expect the new probe.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/awf_helpers.go | Updates the TCP DOCKER_HOST detection regex (and comment rationale) used when emitting the runtime --docker-host-path-prefix probe. |
| pkg/workflow/firewall_args_test.go | Adjusts the unit test expectation to match the new ^tcp:// probe string in generated step content. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| initSnippet := `GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS=""` | ||
| conditionSnippet := `if [[ "${DOCKER_HOST:-}" =~ ^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$ ]]; then` | ||
| conditionSnippet := `if [[ "${DOCKER_HOST:-}" =~ ^tcp:// ]]; then` | ||
| flagAssignmentSnippet := `GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS="--docker-host-path-prefix /tmp/gh-aw"` | ||
| argsRefSnippet := `${GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS}` |
This comment has been minimized.
This comment has been minimized.
|
@copilot review the generated comments please |
salmanmkc
force-pushed
the
salmanmkc/arc-dind-broaden-detection
branch
from
c564dda to
b8e82e8
Compare
|
@copilot review the comments, i think i addressed it already though |
This comment has been minimized.
This comment has been minimized.
Confirmed - the review feedback has been addressed. Both occurrences of the DOCKER_HOST regex in the test file (lines 48 and 248) have been updated from the localhost-only pattern to |
|
@copilot merge main and recompile |
The previous regex only matched tcp://localhost and tcp://127.0.0.1, missing ARC/DinD setups that use K8s service names (tcp://dind:2375), pod IPs, or other TCP Docker daemon configurations. Any tcp:// host indicates the daemon filesystem is separate from the runner, so --docker-host-path-prefix is always needed. Broadens the regex from ^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$ to ^tcp:// to cover all TCP Docker daemon configurations. Ref #28888, #30838, #30840
salmanmkc
force-pushed
the
salmanmkc/arc-dind-broaden-detection
branch
from
9ff0891 to
3c60e49
Compare
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged main and recompiled all 225 workflow lock files. |
|
@copilot fix merge conflicts |
Co-authored-by: salmanmkc <32169182+salmanmkc@users.noreply.github.com>
Co-authored-by: salmanmkc <32169182+salmanmkc@users.noreply.github.com>
![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 /diagnose and /tdd — this is a targeted bug fix with a regression test.
Key Themes
- Test names vs. behavior specs (minor): Several test-case names repeat the raw input value rather than describing the specified behavior. TDD encourages names that read as specifications.
- Two-pass
fmt.Sprintftemplate (minor): ThescriptTemplateconstruction works today, but embedding the regex via%%sis fragile if the constant ever gains%characters. - Boundary documentation (minor): The bare `(redacted) case (matched by the new regex) is not covered by a test case, leaving its intent implicit.
Positive Highlights
- ✅ Minimal, surgical fix — one constant changed, one clear comment explaining the rationale.
- ✅ Excellent regression test approach —
TestArcDindDockerHostDetectionruns the emitted snippet in a realbashsubprocess, testing actual runtime behavior rather than a Go simulation. This is exactly the right seam per/diagnosePhase 5. - ✅ Good coverage of newly-supported patterns — localhost, loopback, K8s service names, and pod IPs are all explicitly tested.
- ✅ PR description is clear — root cause, rationale, and issue references are all present.
Verdict
Approving. The core fix is correct and well-justified, and the new test gives a solid regression guard. The inline comments are non-blocking suggestions for polish.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 7.4M
| tests := []struct { | ||
| name string | ||
| dockerHost string | ||
| wantSet bool |
There was a problem hiding this comment.
[/tdd] Test names here repeat the input value rather than describing the behavior being specified. Per TDD's principle that tests should read as specifications, names like "localhost TCP", "K8s service name (dind)", "pod IP" communicate intent more clearly than "(localhost/redacted) and "(dind/redacted) (K8s service name)"`. A name like the latter is halfway between — pick one style consistently.
| echo "$GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS" | ||
| `, awfArcDindDockerHostRegex, awfArcDindHostPathPrefixFlag) | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
[/diagnose] The scriptTemplate is built with fmt.Sprintf by interpolating awfArcDindDockerHostRegex and awfArcDindHostPathPrefixFlag directly, then using %%s as a placeholder for the inner DOCKER_HOST substitution. This is clever but fragile: if either constant ever contains % characters, fmt.Sprintf will misinterpret them. Consider using strings.Replace or a named placeholder (e.g., ${DOCKER_HOST_PLACEHOLDER}) to decouple the two substitution passes and make the intent clearer.
| {"tcp://dind:2375 (K8s service name)", "tcp://dind:2375", true}, | ||
| {"tcp://172.30.0.5:2375 (pod IP)", "tcp://172.30.0.5:2375", true}, | ||
| {"tcp://dind-sidecar.default.svc:2376", "tcp://dind-sidecar.default.svc:2376", true}, | ||
| {"unix socket (not tcp)", "unix:///var/run/docker.sock", false}, |
There was a problem hiding this comment.
[/tdd] Consider adding a test case for the bare (redacted) value (no host or port). The broadened regex ^(redacted) would match it, and it's worth making that an explicit, documented decision rather than an implicit side-effect. Even if it never occurs in practice, a test case like {"bare (redacted) (no host)", "(redacted) true} makes the boundary self-documenting.
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView all test classifications
Test Highlights✅
|
|
@copilot review all comments |
All comments have been addressed: ✅ Test inconsistency (copilot-pull-request-reviewer): Both occurrences of the DOCKER_HOST regex updated to ✅ Merge main and recompile: Completed - all 225 workflows recompiled (commit 4dfe147) ✅ Fix merge conflicts: Resolved conflicts in 2 workflow lock files and recompiled (commits dec288e & 7088ba4) Summary of changes:
|
|
@copilot review all comments |
Status unchanged from previous review - all comments remain addressed. No new issues found. |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Branch is out-of-date (mergeStateStatus: DIRTY). Requesting a branch update. [pr-sous-chef] request-branch-update - run: https://github.com/github/gh-aw/actions/runs/25824436959
|
The previous regex only matched
tcp://localhostandtcp://127.0.0.1, missing ARC/DinD setups that use K8s service names (tcp://dind:2375), pod IPs, or other TCP Docker daemon configurations.Any
tcp://DOCKER_HOST indicates the daemon filesystem is separate from the runner, so--docker-host-path-prefixis always needed in that case.Broadens the regex from
^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$to^tcp://.Ref #28888, #30838, #30840