Skip to content

[dead-code] chore: remove dead functions — 1 function removed #39422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jun 16, 2026
+1 −146
Merged

[dead-code] chore: remove dead functions — 1 function removed #39422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5282cfe
chore: remove dead function buildArcDindChrootConfigInjectScript
github-actions[bot] Jun 15, 2026
c014013
Merge branch 'main' into dead-code/remove-buildArcDindChrootConfigInj…
pelikhan Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/workflow/awf_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ type AWFConfigFile struct {
Container *AWFContainerConfig `json:"container,omitempty"`

// Chroot contains chroot execution overrides for split-filesystem ARC/DinD runners.
// This field is not populated at compile time; it is injected at runtime by
// buildArcDindChrootConfigInjectScript when DinD topology is detected.
// This field is not populated at compile time; it is injected at runtime when DinD topology is detected.
Chroot *AWFChrootConfig `json:"chroot,omitempty"`
}

Expand Down
14 changes: 0 additions & 14 deletions pkg/workflow/awf_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,6 @@ func awfSupportsChrootConfig(firewallConfig *FirewallConfig) bool {
// 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://...
Comment on lines 956 to 958
// (see buildArcDindChrootConfigInjectScript for standalone use and tests).
//
// The Python is intentionally kept compact to minimise script size and stay within
// GitHub Actions' 21 KB per-step expression limit.
Expand All @@ -977,16 +976,3 @@ except Exception as e:
raise SystemExit(f"chroot config patch failed: {e}") from e
PY`, awfArcDindChrootBinariesSourcePath, awfArcDindChrootIdentityHome, awfArcDindChrootBinariesSourcePath)
}

// buildArcDindChrootConfigInjectScript returns a standalone bash+Python script that
// patches the AWF config file with chroot.binariesSourcePath and chroot.identity.* when the
// runner is in an ARC/DinD topology (detected via DOCKER_HOST=tcp://...).
//
// This standalone form is used by tests. In production, the patch body is embedded
// inside the arcDindPrefixProbe if-block (see BuildAWFCommand) so there is only one
// DOCKER_HOST condition check in the generated script.
func buildArcDindChrootConfigInjectScript() string {
return fmt.Sprintf(`if [[ "${DOCKER_HOST:-}" =~ %s ]]; then
%s
fi`, awfArcDindDockerHostRegex, buildArcDindChrootConfigPatchBody())
}
130 changes: 0 additions & 130 deletions pkg/workflow/awf_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
package workflow

import (
"encoding/json"
"errors"
"fmt"
"os"
"os/exec"
"strconv"
"strings"
Expand Down Expand Up @@ -1463,133 +1460,6 @@ func TestAWFSupportsChrootConfig(t *testing.T) {
}
}

// TestArcDindChrootConfigInjection exercises the Python script generated by
// buildArcDindChrootConfigInjectScript. It writes a minimal AWF config JSON,
// runs the script with a controlled DOCKER_HOST, and verifies that the chroot
// section is added with the expected static paths and runtime identity values.
func TestArcDindChrootConfigInjection(t *testing.T) {
if _, err := exec.LookPath("python3"); err != nil {
t.Skip("python3 not available")
}
if _, err := exec.LookPath("id"); err != nil {
t.Skip("id command not available")
}

tests := []struct {
name string
dockerHost string
wantChroot bool
}{
{"tcp://localhost:2375 triggers injection", "tcp://localhost:2375", true},
{"tcp://dind:2375 triggers injection", "tcp://dind:2375", true},
{"tcp://172.30.0.5:2376 triggers injection", "tcp://172.30.0.5:2376", true},
{"unix socket skips injection", "unix:///var/run/docker.sock", false},
{"bare path skips injection", "/var/run/docker.sock", false},
{"empty DOCKER_HOST skips injection", "", false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tmpDir := t.TempDir()
// The Python script reads from ${RUNNER_TEMP}/gh-aw/awf-config.json,
// so create the gh-aw subdirectory and write the config there.
ghAwDir := tmpDir + "/gh-aw"
configPath := ghAwDir + "/awf-config.json"

// Write a minimal AWF config file (simulating the printf step).
// Use json.Marshal of a real AWFConfigFile to stay in sync with the struct.
minimalConfig := &AWFConfigFile{
APIProxy: &AWFAPIProxyConfig{
Enabled: true,
MaxRuns: 100,
},
}
initialConfigBytes, err := json.Marshal(minimalConfig)
require.NoError(t, err, "failed to marshal minimal AWF config")
initialConfig := string(initialConfigBytes)
writeScript := fmt.Sprintf(`mkdir -p %s && printf '%%s\n' '%s' > %s`, ghAwDir, initialConfig, configPath)

script := buildArcDindChrootConfigInjectScript()

// The Python patch also writes to awfArcDindChrootBinariesSourcePath/awf-config.json
// (/tmp/gh-aw/awf-config.json). Pre-create that directory so the write succeeds,
// and schedule cleanup of the file after the test.
if tt.wantChroot {
if err := os.MkdirAll(awfArcDindChrootBinariesSourcePath, 0o755); err != nil {
t.Skipf("cannot create %s: %v", awfArcDindChrootBinariesSourcePath, err)
}
t.Cleanup(func() { os.Remove(awfArcDindChrootBinariesSourcePath + "/awf-config.json") })
}

fullScript := fmt.Sprintf(`#!/bin/bash
export RUNNER_TEMP=%q
export DOCKER_HOST=%q
%s
%s
cat %s
`, tmpDir, tt.dockerHost, writeScript, script, configPath)

cmd := exec.Command("bash", "-c", fullScript)
out, err := cmd.Output()
var stderrMsg string
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
stderrMsg = string(exitErr.Stderr)
}
require.NoError(t, err, "script should succeed (stderr: %s)", stderrMsg)

var result map[string]any
require.NoError(t, json.Unmarshal(out, &result), "output must be valid JSON: %s", string(out))

if !tt.wantChroot {
assert.NotContains(t, string(out), `"chroot"`,
"chroot section should NOT be injected for DOCKER_HOST=%s", tt.dockerHost)
return
}

require.Contains(t, string(out), `"chroot"`,
"chroot section should be injected for DOCKER_HOST=%s", tt.dockerHost)

// Verify /tmp/gh-aw/awf-config.json was also written (matches the runner_temp copy).
tmpConfigBytes, readErr := os.ReadFile(awfArcDindChrootBinariesSourcePath + "/awf-config.json")
require.NoError(t, readErr, "%s/awf-config.json should have been written by inject script", awfArcDindChrootBinariesSourcePath)
assert.Equal(t, string(out), string(tmpConfigBytes),
"%s/awf-config.json should match %s/gh-aw/awf-config.json", awfArcDindChrootBinariesSourcePath, tmpDir)

chrootRaw, ok := result["chroot"].(map[string]any)
require.True(t, ok, "chroot must be an object")

// Verify binariesSourcePath is the expected constant.
assert.Equal(t, awfArcDindChrootBinariesSourcePath, chrootRaw["binariesSourcePath"],
"binariesSourcePath should be %s", awfArcDindChrootBinariesSourcePath)

// Verify identity section is present.
identityRaw, ok := chrootRaw["identity"].(map[string]any)
require.True(t, ok, "chroot.identity must be an object")

// Verify home is the expected constant.
assert.Equal(t, awfArcDindChrootIdentityHome, identityRaw["home"],
"identity.home should be %s", awfArcDindChrootIdentityHome)

// Verify user is a non-empty string.
user, ok := identityRaw["user"].(string)
assert.True(t, ok && user != "", "identity.user should be a non-empty string")

// Verify uid/gid are positive numbers.
uid, ok := identityRaw["uid"].(float64)
assert.True(t, ok && uid > 0, "identity.uid should be a positive number, got %v", identityRaw["uid"])

gid, ok := identityRaw["gid"].(float64)
assert.True(t, ok && gid > 0, "identity.gid should be a positive number, got %v", identityRaw["gid"])

// Verify original config fields survived the patch (round-trip correctness).
apiProxy, ok := result["apiProxy"].(map[string]any)
require.True(t, ok, "apiProxy section must be preserved after chroot injection")
assert.Equal(t, true, apiProxy["enabled"], "apiProxy.enabled should be unchanged after chroot injection")
})
}
}

// TestBuildAWFCommand_IncludesChrootInjectScript verifies that BuildAWFCommand

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.json and awfArcDindChrootBinariesSourcePath

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.

// includes the chroot injection script in the generated run step when the AWF
// version supports it.
Comment on lines 1463 to 1465
Expand Down
Loading