solve issue related to validate ssh with tunnels #38
Conversation
|
hi @samiulhq sorry for the late reply. Hope this PR fix the issue you meet. |
|
Code Coverage SummaryDiff against mainResults for commit: 8e0c701 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
R/utils.R
Outdated
| @@ -34,9 +34,11 @@ install_saspy <- function(method = "auto", conda = "auto") { | |||
| #' | |||
| #' @keywords internal | |||
| validate_ssh_with_tunnel <- function(session, msg = "SAS session through ssh must use tunnels to transfer datasets!") { | |||
| cfgname <- session$sascfg$SAScfg$SAS_config_names[1] | |||
| cfgname <- session$sascfg$name | |||
| is_ssh <- identical(session$sascfg$mode, "ssh") | |||
There was a problem hiding this comment.
I think identical() will only be TRUE if you match case. So use "SSH" for the literal, as the mode attribute is upper cased in saspy.
Out of curiosity, why require tunneling with the SSH access method? It seems that any connection that's established via SASPy works, so why require this in the first place? I don't have an R install that works with this, so I haven't been able to try it out to see if I can answer that question myself, but it seems that this shouldn't care how saspy is connected; just that it is.
Thanks,
Tom
There was a problem hiding this comment.
thank you Tom. indeed this is upper case. the reason I add it is that a while ago I experienced issues transfering data for ssh without tunneling, but it seems it is working as expected now.
thank you so much!
There was a problem hiding this comment.
oh on some devices where the client is not reachable from the server, tunnelling is still needed. but it is true that this assertion is not needed because users are essentially responsible for the connection. Thank you again!
There was a problem hiding this comment.
I understand. And yes, I agree, if you let users be responsible for configuring access, however they need to, then this package doesn't need to care and would work with any connection! Let me know if there's anything else I can help with!
Thanks,
Tom
There was a problem hiding this comment.
Hey, I'm not expecting I can review these, such that it triggers the merge; says only reviewers w/ write permission, which I assume I'm not. But, adding this review for info anyway. This all looks good to me; I can't test it all out, but looking at the diffs, I don't see anything logically wrong. Looks like you're just allowing any valid connection, which is the right thing.
Don't know if this will trigger anything, but maybe helps with a reviewer who can!
Tom
thank you Tom for your review! |
|
I hope you weren't waiting on me, I didn't think to review it, just cuz it wouldn't merge. Thanks for fixing/enhancing this! |
close #37