Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Support SSH Agent Auth on Windows #405

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

ekyoung
Copy link
Contributor

@ekyoung ekyoung commented May 28, 2017

Fixes #404. I used xanzy/ssh-agent to create the ssh agent correctly based on os based on this PR in terraform that fixed a similar issue in that product.

hashicorp/terraform#4323

@smola smola added the windows label May 29, 2017
@smola smola requested review from mcuadros and alcortesm May 29, 2017 10:13
@smola
Copy link
Collaborator

smola commented May 29, 2017

@ekyoung Thank you for the PR.
Let's get some other team members to approve this, given that it adds a new external dependency (which we usually try to limit).

"os"
"os/user"
"path/filepath"

"github.com/xanzy/ssh-agent"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this import in the public packages group?

@ekyoung
Copy link
Contributor Author

ekyoung commented May 30, 2017

@mcuadros I moved the import statement.

@@ -94,18 +94,9 @@ func (s *SuiteCommon) TestPublicKeysCallbackString(c *C) {
c.Assert(a.String(), Equals, fmt.Sprintf("user: test, name: %s", PublicKeysCallbackName))
}
func (s *SuiteCommon) TestNewSSHAgentAuth(c *C) {
addr := os.Getenv("SSH_AUTH_SOCK")
err := os.Unsetenv("SSH_AUTH_SOCK")
auth, err := NewSSHAgentAuth("foo")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should be skipped if SSH_AUTH_SOCK is not defined:

os.Getenv("SSH_AUTH_SOCK") == "" {
                c.Skip("SSH_AUTH_SOCK or SSH_TEST_PRIVATE_KEY are required")
                return
        }

Also, the old test tested that the right error was returned when agent is not running, please, preserve that test case.

@mcuadros
Copy link
Contributor

mcuadros commented Jun 1, 2017

@ekyoung can you squash your commit to merge this?

@smola smola force-pushed the support-ssh-agent-on-windows branch from 1107000 to f9dc7b1 Compare June 1, 2017 14:57
@codecov
Copy link

codecov bot commented Jun 1, 2017

Codecov Report

Merging #405 into master will decrease coverage by 0.6%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
- Coverage   77.69%   77.08%   -0.61%     
==========================================
  Files         124      124              
  Lines        9001     8998       -3     
==========================================
- Hits         6993     6936      -57     
- Misses       1234     1302      +68     
+ Partials      774      760      -14
Impacted Files Coverage Δ
plumbing/transport/ssh/auth_method.go 33.33% <66.66%> (-23.15%) ⬇️
plumbing/transport/ssh/common.go 0% <0%> (-50.91%) ⬇️
plumbing/transport/http/upload_pack.go 73.95% <0%> (-3.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e249df...f9dc7b1. Read the comment docs.

@smola
Copy link
Collaborator

smola commented Jun 1, 2017

@mcuadros Done.

@smola smola merged commit 87d2475 into src-d:master Jun 1, 2017
@ekyoung
Copy link
Contributor Author

ekyoung commented Jun 1, 2017

Looks like you guys wrapped this up. Sorry I couldn't get back on it earlier. Thanks for merging.

@ekyoung ekyoung deleted the support-ssh-agent-on-windows branch June 1, 2017 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error Using SSH Agent Auth on Windows
4 participants