mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
ci: add Git usr/bin to PATH on Windows (#25939)
## Summary Fixes all 9 Windows CI test failures caused by the mise CI refactor (`fe257666d7`, PR #25727). ### Root cause `jdx/mise-action` exports `Path` (Windows convention) via `GITHUB_ENV`. Bash on Windows maintains its own `PATH`. When Go's `os.Environ()` returns both, `cmd.exe` subprocesses non-deterministically pick the MSYS-translated `PATH` (forward slashes), causing Windows executables (`printf`, `powershell.exe`, `cmd.exe`) to be unresolvable. These failures only appeared on `main` (where `-count=1` forces real test execution) and were masked on PRs by Go test cache. ### Fixes applied **CI (`setup-mise` action)**: - Write both `Path` and `PATH` to `GITHUB_ENV` with Git usr/bin prepended **Code (`cli/root.go`)**: - Add `appendAndDedupEnv` helper that deduplicates case-insensitive env vars on Windows, preferring native Windows paths (backslashes) over MSYS paths **Code (`cli/configssh_windows.go`)**: - Use absolute paths for `powershell.exe` and `cmd.exe` in the SSH config `Match exec` escape function, avoiding PATH resolution entirely **Tests**: - Switch `--header-command` tests from `printf` to `echo` (cmd.exe builtin) for reliable cross-platform execution - Add env dedup in `Test_sshConfigMatchExecEscape` for subprocess PATH consistency Fixes coder/internal#1556, coder/internal#1558, coder/internal#1559 > 🤖 Generated by Coder agent, will be reviewed by @mafredri. 🏂🏻
This commit is contained in:
committed by
GitHub
parent
fc01aeeb0f
commit
ed4311b2cb
@@ -166,3 +166,23 @@ runs:
|
|||||||
mise_dir: ${{ steps.mise-data-dir.outputs.path }}
|
mise_dir: ${{ steps.mise-data-dir.outputs.path }}
|
||||||
install_args: ${{ steps.cache-key.outputs.install-args }}
|
install_args: ${{ steps.cache-key.outputs.install-args }}
|
||||||
cache: "false"
|
cache: "false"
|
||||||
|
|
||||||
|
- name: Ensure Git usr/bin is in PATH (Windows)
|
||||||
|
if: runner.os == 'Windows'
|
||||||
|
shell: pwsh
|
||||||
|
# jdx/mise-action exports "Path" via GITHUB_ENV which may
|
||||||
|
# collide with bash's "PATH". Ensure Git usr/bin is present
|
||||||
|
# and remove any duplicate Path/PATH entries from GITHUB_ENV
|
||||||
|
# by writing both forms.
|
||||||
|
run: | # zizmor: ignore[github-env]
|
||||||
|
$gitdir = "C:\Program Files\Git\usr\bin"
|
||||||
|
$current = $env:Path
|
||||||
|
if ($current -notlike "*$gitdir*") {
|
||||||
|
$current = "$gitdir;$current"
|
||||||
|
}
|
||||||
|
# Write both Path and PATH to GITHUB_ENV so that both
|
||||||
|
# cmd.exe (uses Path) and bash/Go (uses PATH) see the
|
||||||
|
# same value including Git usr/bin.
|
||||||
|
"Path=$current" | Out-File -Append -FilePath $env:GITHUB_ENV -Encoding utf8
|
||||||
|
"PATH=$current" | Out-File -Append -FilePath $env:GITHUB_ENV -Encoding utf8
|
||||||
|
|
||||||
|
|||||||
+3
-1
@@ -146,8 +146,10 @@ func TestWorkspaceAgent(t *testing.T) {
|
|||||||
}).WithAgent().Do()
|
}).WithAgent().Do()
|
||||||
|
|
||||||
coderURLEnv := "$CODER_URL"
|
coderURLEnv := "$CODER_URL"
|
||||||
|
headerCmd := "printf X-Process-Testing=very-wow-" + coderURLEnv + "'\\r\\n'X-Process-Testing2=more-wow"
|
||||||
if runtime.GOOS == "windows" {
|
if runtime.GOOS == "windows" {
|
||||||
coderURLEnv = "%CODER_URL%"
|
coderURLEnv = "%CODER_URL%"
|
||||||
|
headerCmd = "echo X-Process-Testing=very-wow-" + coderURLEnv + "& echo X-Process-Testing2=more-wow"
|
||||||
}
|
}
|
||||||
|
|
||||||
logDir := t.TempDir()
|
logDir := t.TempDir()
|
||||||
@@ -159,7 +161,7 @@ func TestWorkspaceAgent(t *testing.T) {
|
|||||||
"--log-dir", logDir,
|
"--log-dir", logDir,
|
||||||
"--agent-header", "X-Testing=agent",
|
"--agent-header", "X-Testing=agent",
|
||||||
"--agent-header", "Cool-Header=Ethan was Here!",
|
"--agent-header", "Cool-Header=Ethan was Here!",
|
||||||
"--agent-header-command", "printf X-Process-Testing=very-wow-"+coderURLEnv+"'\\r\\n'X-Process-Testing2=more-wow",
|
"--agent-header-command", headerCmd,
|
||||||
"--socket-path", testutil.AgentSocketPath(t),
|
"--socket-path", testutil.AgentSocketPath(t),
|
||||||
)
|
)
|
||||||
clitest.Start(t, agentInv)
|
clitest.Start(t, agentInv)
|
||||||
|
|||||||
@@ -229,8 +229,15 @@ func Test_sshConfigMatchExecEscape(t *testing.T) {
|
|||||||
|
|
||||||
// OpenSSH processes %% escape sequences into %
|
// OpenSSH processes %% escape sequences into %
|
||||||
escaped = strings.ReplaceAll(escaped, "%%", "%")
|
escaped = strings.ReplaceAll(escaped, "%%", "%")
|
||||||
b, err := exec.Command(cmd, arg, escaped).CombinedOutput() //nolint:gosec
|
c := exec.Command(cmd, arg, escaped) //nolint:gosec
|
||||||
require.NoError(t, err)
|
if runtime.GOOS == "windows" {
|
||||||
|
// Deduplicate Path/PATH env vars so cmd.exe
|
||||||
|
// subprocesses (like powershell.exe used for
|
||||||
|
// paths with spaces) resolve correctly.
|
||||||
|
c.Env = appendAndDedupEnv(os.Environ())
|
||||||
|
}
|
||||||
|
b, err := c.CombinedOutput()
|
||||||
|
require.NoError(t, err, "command output: %s", string(b))
|
||||||
got := strings.TrimSpace(string(b))
|
got := strings.TrimSpace(string(b))
|
||||||
require.Equal(t, "yay", got)
|
require.Equal(t, "yay", got)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -4,6 +4,8 @@ package cli
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"os"
|
||||||
|
"path/filepath"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"golang.org/x/xerrors"
|
"golang.org/x/xerrors"
|
||||||
@@ -50,7 +52,13 @@ func sshConfigMatchExecEscape(path string) (string, error) {
|
|||||||
|
|
||||||
if strings.ContainsAny(path, " ") {
|
if strings.ContainsAny(path, " ") {
|
||||||
// c.f. function comment for how this works.
|
// c.f. function comment for how this works.
|
||||||
path = fmt.Sprintf("for /f %%%%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%%%a%s%%%%a", path) //nolint:gocritic // We don't want %q here.
|
// Use absolute paths for powershell.exe and cmd.exe
|
||||||
|
// to avoid PATH resolution issues when both Path and
|
||||||
|
// PATH (MSYS-translated) exist in the environment.
|
||||||
|
sysRoot := os.Getenv("SYSTEMROOT")
|
||||||
|
pwsh := filepath.Join(sysRoot, "System32", "WindowsPowerShell", "v1.0", "powershell.exe")
|
||||||
|
cmd := filepath.Join(sysRoot, "System32", "cmd.exe")
|
||||||
|
path = fmt.Sprintf("for /f %%%%a in ('%s -Command [char]34') do @%s /c %%%%a%s%%%%a", pwsh, cmd, path) //nolint:gocritic // We don't want %q here.
|
||||||
}
|
}
|
||||||
return path, nil
|
return path, nil
|
||||||
}
|
}
|
||||||
|
|||||||
+39
-2
@@ -1701,7 +1701,44 @@ func (r roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
|
|||||||
return r(req)
|
return r(req)
|
||||||
}
|
}
|
||||||
|
|
||||||
// HeaderTransport creates a new transport that executes `--header-command`
|
// appendAndDedupEnv appends extra environment variables and
|
||||||
|
// deduplicates entries with the same key (case-insensitive on
|
||||||
|
// Windows). For the PATH variable specifically, it prefers the
|
||||||
|
// value that contains native Windows paths (with backslashes)
|
||||||
|
// over MSYS-translated paths (with forward slashes). For all
|
||||||
|
// other variables, the last value wins.
|
||||||
|
func appendAndDedupEnv(env []string, extra ...string) []string {
|
||||||
|
env = append(env, extra...)
|
||||||
|
if runtime.GOOS != "windows" {
|
||||||
|
return env
|
||||||
|
}
|
||||||
|
seen := make(map[string]int, len(env))
|
||||||
|
result := make([]string, 0, len(env))
|
||||||
|
for _, e := range env {
|
||||||
|
key, val, ok := strings.Cut(e, "=")
|
||||||
|
if !ok {
|
||||||
|
result = append(result, e)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
upper := strings.ToUpper(key)
|
||||||
|
if idx, exists := seen[upper]; exists {
|
||||||
|
if upper == "PATH" {
|
||||||
|
// Prefer the value with native Windows paths.
|
||||||
|
existingVal := result[idx][len(key)+1:]
|
||||||
|
if strings.Contains(existingVal, "\\") && !strings.Contains(val, "\\") {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
}
|
||||||
|
result[idx] = e
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
seen[upper] = len(result)
|
||||||
|
result = append(result, e)
|
||||||
|
}
|
||||||
|
return result
|
||||||
|
}
|
||||||
|
|
||||||
|
// headerTransport creates a new transport that executes `--header-command`
|
||||||
// if it is set to add headers for all outbound requests.
|
// if it is set to add headers for all outbound requests.
|
||||||
func headerTransport(ctx context.Context, serverURL *url.URL, header []string, headerCommand string) (*codersdk.HeaderTransport, error) {
|
func headerTransport(ctx context.Context, serverURL *url.URL, header []string, headerCommand string) (*codersdk.HeaderTransport, error) {
|
||||||
transport := &codersdk.HeaderTransport{
|
transport := &codersdk.HeaderTransport{
|
||||||
@@ -1719,7 +1756,7 @@ func headerTransport(ctx context.Context, serverURL *url.URL, header []string, h
|
|||||||
var outBuf bytes.Buffer
|
var outBuf bytes.Buffer
|
||||||
// #nosec
|
// #nosec
|
||||||
cmd := exec.CommandContext(ctx, shell, caller, headerCommand)
|
cmd := exec.CommandContext(ctx, shell, caller, headerCommand)
|
||||||
cmd.Env = append(os.Environ(), "CODER_URL="+serverURL.String())
|
cmd.Env = appendAndDedupEnv(os.Environ(), "CODER_URL="+serverURL.String())
|
||||||
cmd.Stdout = &outBuf
|
cmd.Stdout = &outBuf
|
||||||
cmd.Stderr = io.Discard
|
cmd.Stderr = io.Discard
|
||||||
err := cmd.Run()
|
err := cmd.Run()
|
||||||
|
|||||||
+4
-2
@@ -177,15 +177,17 @@ func TestRoot(t *testing.T) {
|
|||||||
url = srv.URL
|
url = srv.URL
|
||||||
buf := new(bytes.Buffer)
|
buf := new(bytes.Buffer)
|
||||||
coderURLEnv := "$CODER_URL"
|
coderURLEnv := "$CODER_URL"
|
||||||
|
headerCmd := "printf X-Process-Testing=very-wow-" + coderURLEnv + "'\\r\\n'X-Process-Testing2=more-wow"
|
||||||
if runtime.GOOS == "windows" {
|
if runtime.GOOS == "windows" {
|
||||||
coderURLEnv = "%CODER_URL%"
|
coderURLEnv = "%CODER_URL%"
|
||||||
|
headerCmd = "echo X-Process-Testing=very-wow-" + coderURLEnv + "& echo X-Process-Testing2=more-wow"
|
||||||
}
|
}
|
||||||
inv, _ := clitest.New(t,
|
inv, _ := clitest.New(t,
|
||||||
"--no-feature-warning",
|
"--no-feature-warning",
|
||||||
"--no-version-warning",
|
"--no-version-warning",
|
||||||
"--header", "X-Testing=wow",
|
"--header", "X-Testing=wow",
|
||||||
"--header", "Cool-Header=Dean was Here!",
|
"--header", "Cool-Header=Dean was Here!",
|
||||||
"--header-command", "printf X-Process-Testing=very-wow-"+coderURLEnv+"'\\r\\n'X-Process-Testing2=more-wow",
|
"--header-command", headerCmd,
|
||||||
"login", srv.URL,
|
"login", srv.URL,
|
||||||
)
|
)
|
||||||
inv.Stdout = buf
|
inv.Stdout = buf
|
||||||
@@ -266,7 +268,7 @@ func TestDERPHeaders(t *testing.T) {
|
|||||||
"--no-version-warning",
|
"--no-version-warning",
|
||||||
"ping", workspace.Name,
|
"ping", workspace.Name,
|
||||||
"-n", "1",
|
"-n", "1",
|
||||||
"--header-command", "printf X-Process-Testing=very-wow",
|
"--header-command", "echo X-Process-Testing=very-wow",
|
||||||
}
|
}
|
||||||
for k, v := range expectedHeaders {
|
for k, v := range expectedHeaders {
|
||||||
if k != "X-Process-Testing" {
|
if k != "X-Process-Testing" {
|
||||||
|
|||||||
@@ -48,7 +48,7 @@ func Test_ProxyServer_Headers(t *testing.T) {
|
|||||||
"--access-url", "http://localhost:8080",
|
"--access-url", "http://localhost:8080",
|
||||||
"--http-address", ":0",
|
"--http-address", ":0",
|
||||||
"--header", fmt.Sprintf("%s=%s", headerName1, headerVal1),
|
"--header", fmt.Sprintf("%s=%s", headerName1, headerVal1),
|
||||||
"--header-command", fmt.Sprintf("printf %s=%s", headerName2, headerVal2),
|
"--header-command", fmt.Sprintf("echo %s=%s", headerName2, headerVal2),
|
||||||
)
|
)
|
||||||
pty := ptytest.New(t)
|
pty := ptytest.New(t)
|
||||||
inv.Stdout = pty.Output()
|
inv.Stdout = pty.Output()
|
||||||
|
|||||||
Reference in New Issue
Block a user