-
Notifications
You must be signed in to change notification settings - Fork 76
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
Merge #285 for default authentication support #286
Comments
I think you are still missing the part where we suggest to file an issue ticket to establish what should be in a pull request before submitting one. |
As @randomee notes in the PR, this change is motivated by a comment I made in #251 long ago. It's a lot to process all at once, but I am following most of it and think it generally works. On the issue of appropriate behavior given conflicting options: I don't think we should override the meaning of |
Thanks for having a look John. I'm more than happy to explain anything that doesn't immediately make sense. And I'm happy to test/edit as needed as well. The current code does what you would like wrt auto auth w/o auto connect. It could be made more clean if that's desired.
|
Updated to merge to current head (conflicts arose from the bpipe commits). reverted the ip.address change that John pointed to being problematic. tested new code from linux against
Alas, I don't have a bpipe to test against. Happy to discuss, or make changes as requested. |
@alfredkanzler Could you possibly check if there are any side effects with bpipe? |
I have not noticed any side effects. Are there specific problems you have seen?
On May 20, 2019, at 11:06, Dirk Eddelbuettel <notifications@github.com<mailto:notifications@github.com>> wrote:
@alfredkanzler<https://github.com/alfredkanzler> Could you possibly check if there are any side effects with bpipe?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#286?email_source=notifications&email_token=ALFF6ERD2KOQWK3LMYLKEP3PWLEBFA5CNFSM4GN2KDVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVZJ6WA#issuecomment-494051160>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ALFF6ETWS2KY3UE5PONTQYLPWLEBFANCNFSM4GN2KDVA>.
This e-mail and any attachments may contain information that is confidential and proprietary and otherwise protected from disclosure. If you are not the intended recipient of this e-mail, do not read, duplicate or redistribute it by any means. Please immediately delete it and any attachments and notify the sender that you have received it by mistake. Unintended recipients are prohibited from taking action on the basis of information in this e-mail or any attachments. The DRW Companies make no representations that this e-mail or any attachments are free of computer viruses or other defects.
|
Well I can't currently run it so I rely on all of you in the crew to tell me if something where to be fishy. This looked clean from glancing at the diff though... |
I recently had a conversation with a user in Canada. He was having problems because he did not set his identity. I sent him a code snippet and he was able to successfully run bpipe. He did not seem to have any side effects. I also have not noticed any side effects. I am also running this is a shiny application and it seems to run without issues. Is there and issue with the original path. I will try running on Windows tomorrow with the original path. |
I just ran the bdh on the original path and through bpipe. Both ran successfully. |
Mostly a discussion point (per Dirk) for #285
Explanation of what's changed... It's very, very, very similar to the existing default
con
code.Change default args to
blpAuthenticate()
to support usegetOptions()
for args.Change
blpAuthenticate()
to store the resultingidentity
to.pkgenv$blpAuth
Added one file/function -
defaultAuthentication()
which is more or less a copy/paste of defaultConnection(). But nostop()
if no identity object is found. Since you don't need to have an identity for all connections. NULL is fine for desktop, and even some SAPI serversChanged the defaults for some "data" functions (e.g.
bds() bdp() subscribe()
to useidentity = defaultAuthentication()
instead ofidentity = NULL
Added
defaultAuthentication()
to NAMESPACE.Added documentation to vignette and to the function files.
The only outstanding bit is some of the data functions don't have support for
identity
(see #284 ). Once they do, they only need to useidentity = defaultAuthentication()
akin to the other data functions.It's arguable where
blpAutoAuthenticate
should fit/nest ininit.R
I thinkblpAutoAuthenticate
should be dependent onblpAuthConnect
. It doesn't make any sense to authenticate w/o a connection. It will always fail. I think settingblpAutoAuthenticate
should auto-setblpAuthConnect
. But I'm good however.Tested from Linux (Ubuntu 18.04LTS) against a desktop (DAPI) host, and a SAPI host. I don't have access to B-PIPE, but it should work since nothing changed on the C++/lib end.
Example of the code working
Connecting w/o auto connection/auth
Connecting to a desktop (no authentication needed) server (old codepath)
The text was updated successfully, but these errors were encountered: