Auto-detect ARC/DinD and emit AWF --docker-host-path-prefix in generated workflows#31614
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
--docker-host-path-prefix in generated workflows
|
@copilot merge main and recompile |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.3M
| } | ||
|
|
||
| // Build the expandable args string for args that need shell variable expansion. | ||
| // These MUST be appended as raw (unescaped) strings because single-quoting would |
There was a problem hiding this comment.
[/zoom-out] The arcDindPrefixProbe injection is copy-pasted into all four fmt.Sprintf branches (lines ~193, ~212, ~229, ~245). These branches already differ only in which of pathSetup / preCreateLog / configFileSetup they include — this change makes each branch one line longer in two places, amplifying the existing duplication.
Consider extracting the command assembly into a small helper that accepts the optional preamble lines as a slice and joins them, so the ARC/DinD probe (and any future injections) only needs to be added once:
func assemblePipefailCommand(preambles []string, probe, awfCmd, expandableArgs, arcDindRef, awfArgs, wrapped, logFile string) string {
body := strings.Join(append(preambles, probe), "\n")
return fmt.Sprintf("set -o pipefail\n%s\n# shellcheck disable=SC1003\n%s %s %s %s \\\n -- %s 2>&1 | tee -a %s", body, awfCmd, expandableArgs, arcDindRef, awfArgs, wrapped, logFile)
}This would make future version-gated injections trivial and keep the logic in one place.
| if awfSupportsDockerHostPathPrefix(firewallConfig) { | ||
| arcDindPrefixProbe = fmt.Sprintf(`%s="" | ||
| if [[ "${DOCKER_HOST:-}" =~ %s ]]; then | ||
| %s="%s" |
There was a problem hiding this comment.
[/tdd] When awfSupportsDockerHostPathPrefix returns false, arcDindPrefixProbe is set to "" and arcDindPrefixArgsRef is also "". Each format string has %s\n%s\n — two consecutive %s slots that will both expand to empty strings, producing a stray blank line in the generated script on older AWF versions.
A test that captures the full command output for the old-version path and asserts there are no consecutive blank lines (or that the probe variable name does not appear at all) would pin this behavior explicitly and catch any future regression.
| } | ||
|
|
||
| initSnippet := `GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS=""` | ||
| conditionSnippet := `if [[ "${DOCKER_HOST:-}" =~ ^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$ ]]; then` |
There was a problem hiding this comment.
[/tdd] The conditionSnippet string duplicates the regex literal from the awfArcDindDockerHostRegex package-level constant in awf_helpers.go. If the regex ever changes (e.g., to add IPv6 support), this test will pass with the old string while the real probe uses the new one.
Reference the constant directly so the test always stays in sync:
conditionSnippet := fmt.Sprintf(`if [[ "${DOCKER_HOST:-}" =~ %s ]]; then`, awfArcDindDockerHostRegex)| stepContent := requireCopilotExecutionStep(t, steps) | ||
|
|
||
| if strings.Contains(stepContent, "--docker-host-path-prefix /tmp/gh-aw") { | ||
| t.Error("Expected command to skip --docker-host-path-prefix for unsupported AWF versions") |
There was a problem hiding this comment.
[/tdd] The "skips docker-host-path-prefix probe" test only asserts that --docker-host-path-prefix is absent from the generated command. It doesn't assert that the probe variable initialisation (GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS="") is also absent.
On an old AWF version the probe block should be entirely omitted — not just the flag value. Adding:
if strings.Contains(stepContent, awfArcDindPrefixArgsVarName) {
t.Error("Expected no probe variable for unsupported AWF versions")
}would give the test full coverage of the negative path and guard against accidental partial injection.
Draft ADR generated by the Design Decision Gate covering the runtime DOCKER_HOST probe and version-gated --docker-host-path-prefix emission introduced by this PR.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (142 new lines across AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft covers:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
The runtime DinD probe is exactly the kind of decision worth capturing: it's a load-bearing piece of generated shell that future contributors will need to understand before touching 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs in this repo live in
|
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView All Tests
Analysis Notes✅
|
There was a problem hiding this comment.
Pull request overview
Adds automatic ARC/DinD topology detection to generated AWF-backed workflow commands so users no longer need to manually author --docker-host-path-prefix workarounds, while safely gating emission based on the pinned AWF version.
Changes:
- Emit a runtime Bash probe (based on
DOCKER_HOSTlocalhost TCP patterns) and conditionally append--docker-host-path-prefix /tmp/gh-awto the AWF invocation. - Add a minimum-version constant (
AWFDockerHostPathPrefixMinVersion = v0.25.43) plus a helper gate (awfSupportsDockerHostPathPrefix). - Extend unit tests to validate probe presence/order by default and omission for older pinned AWF versions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/firewall_args_test.go | Adds assertions around the new ARC/DinD probe and version-gated skipping behavior. |
| pkg/workflow/awf_helpers.go | Implements runtime ARC/DinD detection and conditionally injects --docker-host-path-prefix into AWF command generation. |
| pkg/workflow/awf_helpers_test.go | Adds unit coverage for the new version gate helper. |
| pkg/constants/version_constants.go | Introduces the minimum supported AWF version constant for --docker-host-path-prefix. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
| awfArcDindHostPathPrefixFlag) | ||
| arcDindPrefixArgsRef = fmt.Sprintf("${%s}", awfArcDindPrefixArgsVarName) | ||
| } |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all comments |
Done. I synced with |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed all comments and addressed the outstanding test coverage feedback in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I merged |
Document the runtime probe added in #31614 that detects ARC with Docker-in-Docker sidecar topology via DOCKER_HOST and appends --docker-host-path-prefix /tmp/gh-aw to AWF invocations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.