-
Notifications
You must be signed in to change notification settings - Fork 342
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
Support external authentication #390
base: master
Are you sure you want to change the base?
Support external authentication #390
Conversation
@EmielBruijntjes Would you please take a look at this? |
The Address and the Connection classes now use Authentication instead of Login
a59e62f
to
6b35745
Compare
If desired I could change the PR to use less |
I indeed do not like the shared-pointer mechanism, especially not that it is exposed to "user space", that implementation-specific detail (that it uses a shared pointer) should be encapsulated inside AMQP-CPP. |
Okay, the PR is updated to use no |
It seems this is failing in ci because of docker pull rate limiting? @EmielBruijntjes
|
@stertingen would you mind retriggering the CI? |
How would I do this? |
CI might have to be manually triggered by the repo owners. If it is not it might suffice to close the PR and reopening it. The reason I want you to try to get CI to rerun is I think the build failed because of rate limiting on the docker hub. Nothing to do with your change, because your code looks fine. As long as it does not build properly it won't get traction for a merge. Thanks! |
7f0c448
to
4a13ba4
Compare
Great @stertingen! Let's hope @EmielBruijntjes can decide on a "go or no-go" now that it passes the checks! |
One issue I see with this change is that Address already includes an Authentication object (Login), which means that two authentication objects are created when creating a TcpConnection.
Could Address be modified to use Authentication instead, in which case perhaps TcpConnection doesn't need the third parameter? This is one reason why my initial inclination was to modify Login to provide external authentication like pull request #93, and my pull request #458 (which is an alternate but very similar approach). Either way works, it's a question of which is more desirable |
I agree with @trevorperrin. The structure of the code and how the Address, Login and Authentication classes interact needs a bit more attention. |
Authentication
base classConnection
andTcpConnection
classes use newAuthentication
classAuthentication
classThis PR aims to replace #93