-
Notifications
You must be signed in to change notification settings - Fork 6
fix(ssh): normalize Windows ProxyCommand paths #1173
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.
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
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0f81591
fix(ssh): normalize Windows proxy paths
skevetter d4245d2
fix: resolve lint findings
skevetter 61b1cb8
ci: update golangci-lint for Go 1.27
skevetter e17e1c6
ci: pin Go version for lint hooks
skevetter c182761
ci: pin Renovate golangci-lint updates
skevetter 6f15f89
test: use Podman in Windows SSH E2E
skevetter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| package ssh | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/devsy-org/devsy/e2e/framework" | ||
| "github.com/onsi/ginkgo/v2" | ||
| "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| var _ = ginkgo.Describe( | ||
| "devsy Windows SSH ProxyCommand", | ||
| ginkgo.Label("ssh-proxy-command"), | ||
| func() { | ||
| var initialDir string | ||
|
|
||
| ginkgo.BeforeEach(func() { | ||
| var err error | ||
| initialDir, err = os.Getwd() | ||
| framework.ExpectNoError(err) | ||
| }) | ||
|
|
||
| ginkgo.It( | ||
| "should launch a workspace through an executable path containing spaces", | ||
| ginkgo.SpecTimeout(framework.TimeoutLong()), | ||
| func(ctx context.Context) { | ||
| if runtime.GOOS != osWindows { | ||
| ginkgo.Skip("skipping on non-Windows") | ||
| } | ||
|
|
||
| tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| baseFramework := framework.NewDefaultFramework(initialDir + "/bin") | ||
| sourcePath := filepath.Join(baseFramework.DevsyBinDir, baseFramework.DevsyBinName) | ||
| fixtureDir := filepath.Join(ginkgo.GinkgoT().TempDir(), "Devsy Test") | ||
| framework.ExpectNoError(os.MkdirAll(fixtureDir, 0o700)) | ||
| fixturePath := filepath.Join(fixtureDir, baseFramework.DevsyBinName) | ||
| // #nosec G304 -- controlled path to the E2E fixture binary | ||
| binary, err := os.ReadFile(sourcePath) | ||
| framework.ExpectNoError(err) | ||
| framework.ExpectNoError(os.WriteFile(fixturePath, binary, 0o600)) | ||
| gomega.Expect(fixturePath).To(gomega.ContainSubstring(" ")) | ||
|
|
||
| f, err := framework.SetupDockerProvider(fixtureDir, "podman") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| sshConfigPath := filepath.Join(ginkgo.GinkgoT().TempDir(), "ssh config") | ||
| ginkgo.DeferCleanup(func(cleanupCtx context.Context) { | ||
| _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) | ||
| framework.CleanupTempDir(initialDir, tempDir) | ||
| }) | ||
|
|
||
| upCtx, cancelUp := context.WithTimeout(ctx, 5*time.Minute) | ||
| defer cancelUp() | ||
| err = f.DevsyUp(upCtx, tempDir, "--ssh-config", sshConfigPath) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) | ||
| framework.ExpectNoError(err) | ||
| config := string(configBytes) | ||
| expectedPath := strings.ReplaceAll(fixturePath, `\`, "/") | ||
| gomega.Expect(config).To( | ||
| gomega.ContainSubstring(`ProxyCommand "`+expectedPath+`"`), | ||
| "SSH config should use forward slashes for the executable path", | ||
| ) | ||
| gomega.Expect(config).NotTo( | ||
| gomega.ContainSubstring(fixturePath), | ||
| "SSH config should not contain the native Windows executable path", | ||
| ) | ||
|
|
||
| sshPath, err := exec.LookPath("ssh.exe") | ||
| framework.ExpectNoError(err) | ||
| host := filepath.Base(tempDir) + ".devsy" | ||
| sshCtx, cancelSSH := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancelSSH() | ||
| // #nosec G204 -- controlled OpenSSH invocation for the E2E test | ||
| cmd := exec.CommandContext( | ||
| sshCtx, | ||
| sshPath, | ||
| "-F", sshConfigPath, | ||
| "-o", "BatchMode=yes", | ||
| host, | ||
| "printf", | ||
| "proxy-command-ok", | ||
| ) | ||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
| err = cmd.Run() | ||
| framework.ExpectNoError( | ||
| err, | ||
| "OpenSSH should launch ProxyCommand; stdout=%q stderr=%q", | ||
| stdout.String(), stderr.String(), | ||
| ) | ||
| gomega.Expect(strings.TrimSpace(stdout.String())).To( | ||
| gomega.Equal("proxy-command-ok"), | ||
| ) | ||
| }, | ||
| ) | ||
| }, | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| //go:build windows | ||
|
|
||
| package ssh | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestAddHostSectionNormalizesWindowsExecPath(t *testing.T) { | ||
| execPath := `C:\Users\Test User\AppData\Local\Programs\Devsy\resources\bin\devsy.exe` | ||
| result, err := addHostSection("", execPath, addHostParams{ | ||
| host: "testhost", | ||
| user: "ubuntu", | ||
| context: "default", | ||
| workspace: "testworkspace", | ||
| workdir: "/workspaces/project", | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Contains( | ||
| t, | ||
| result, | ||
| `ProxyCommand "C:/Users/Test User/AppData/Local/Programs/Devsy/resources/bin/devsy.exe" workspace ssh --stdio --context default --user ubuntu testworkspace`, | ||
| ) | ||
| require.NotContains(t, result, `C:\Users\Test User`) | ||
| require.Contains(t, result, `--workdir "/workspaces/project"`) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: devsy-org/devsy
Length of output: 7546
🏁 Script executed:
Repository: devsy-org/devsy
Length of output: 3547
🌐 Web query:
Renovate pre-commit manager extract.ts packageName repository URL matchPackageNames💡 Result:
In Renovate, the extraction logic for any manager is handled by its specific
extract.tsfile, which follows a standardized internal interface [1][2]. For thepre-commitmanager, theextract.tsfile is responsible for parsing.pre-commit-config.yamlfiles [3]. It identifies dependencies by extracting their repository URL and revision (tag) [3][4]. ThedepNametypically reflects the repository identifier, while thepackageNameoften represents the full canonical name of the dependency [5][6]. Regarding your query aboutmatchPackageNames: 1. Default Behavior: Historically,matchPackageNamesin Renovate configuration rules has primarily matched against thedepNameof a dependency [5][7]. 2. Distinctions: To address ambiguity betweendepNameandpackageName, Renovate introducedmatchDepNames, which strictly matches only thedepName[5]. 3. Experimental Feature: There is an experimental environment variable,RENOVATE_X_MATCH_PACKAGE_NAMES_MORE, which, when enabled, allowsmatchPackageNamesto also consider thepackageNameduring the matching process [5]. When writing or debugging custom managers or analyzing the built-inpre-commitmanager, ensure you are aware thatextract.tsreturns these fields (depName,packageName,datasource,currentValue) to the worker, which then uses them to evaluate yourpackageRules[8][9][2]. If you are finding thatmatchPackageNamesis not targeting the expected dependency, it is often because the rule is matching thedepNameinstead of thepackageName[5][7].Citations:
🏁 Script executed:
Repository: devsy-org/devsy
Length of output: 3845
🏁 Script executed:
Repository: devsy-org/devsy
Length of output: 4961
Match the Renovate dependency name, not the repository URL.
Renovate extracts this repository as
depNameandpackageName:golangci/golangci-lint. The rule matches the full URL, soallowedVersionsdoes not apply.Change the matcher to
golangci/golangci-lint.🤖 Prompt for AI Agents
Source: MCP tools