-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Test LibGit2 SSH authentication #17651
Conversation
thanks. all of the now-collapsed review from #17586 on the |
Yep |
Rebased, still don't merge though. Now triggers a kernel hang when you attach a debugger. Will try with a more recent kernel to see what I can do. |
b0ad410
to
423bc9d
Compare
Ok, this should be good to go if and when CI passes. |
ssh_repo = joinpath(dir, "Example.SSH") | ||
if !is_windows() | ||
try | ||
sshd_command = strip(readstring(`which sshd`)) |
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.
which
is not always installed. how aboutsuccess(
sshd)
inside a try catch?
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.
That's not the purpose of this command. sshd
refuses to be invoked as anything other than it's absolute path.
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.
oh. add a comment that says that.
Review comments addressed and hopefully added support for the ancient versions of openssh we apparently have on CI. |
What was the change for that? |
Use |
Should Line 83 in bc4eba5
|
Hmm, yeah I suppose it does require a loopback. |
@@ -80,7 +80,7 @@ function choosetests(choices = []) | |||
prepend!(tests, linalgtests) | |||
end | |||
|
|||
net_required_for = ["socket", "parallel"] | |||
net_required_for = ["socket", "parallel", "libgit2"] |
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.
not libgit2
but libgit2-online
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.
No, these tests are in libgit2, since they don't require a network connection but they do require lo
. I suppose we could split them out yet again, but not sure it's worth it. @tkelman?
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.
this seems fine, I rarely see net_on not true. WSL is one of the few places that's the case
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.
World Surf League?
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.
windows subsystem for linux - aka bash on ubuntu on windows
I thought that you always have a loopback on any modern OS. |
this has been failing on the mac buildbots https://build.julialang.org/builders/build_osx10.9-x64/builds/1500/steps/shell_2/logs/stdio edit: and ubuntu https://build.julialang.org/builders/build_ubuntu14.04-x64/builds/1244/steps/shell_2/logs/stdio |
Could potentially be fixed by #17874. |
not fixed by #17874, will prevent new nightlies from being built until working properly https://build.julialang.org/builders/build_centos7.1-x64/builds/1703/steps/shell_2/logs/stdio |
It's not related to #17874. There is something wrong with SSHD key setup.
|
@tkelman can you see if ssh-keygen is working on the buildbots, perhaps we should include ssh-keygen output in the log file? |
Better put a |
-d only allows one connection. See the debug flag that's already in the test |
What should I look for? |
Try the keygen commands in the test file. If that works fine, enable the debug flag. |
(cherry picked from commit 7f074e9)
ref #17651 (comment) (cherry picked from commit 8f06a6c) [av skip]
…ots (#18397) ref #17651 (comment) (cherry picked from commit 8f06a6c) [av skip]
This was part of #17586 (and those commits will show up here until that is merged) and is a WIP attempt at writing a test for all the authentication stuff. Currently flakey, so do not merge until I've had a chance to fix that.