Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Get token from AuthenticateAsync #493

Open
Emdot opened this issue Feb 17, 2021 · 4 comments · May be fixed by #496
Open

Get token from AuthenticateAsync #493

Emdot opened this issue Feb 17, 2021 · 4 comments · May be fixed by #496

Comments

@Emdot
Copy link
Contributor

Emdot commented Feb 17, 2021

What version of Docker.DotNet?:

fork from main as of 2/14/2021 (commit 8e3cf4f)

Problem:
ISystemOperations.AuthenticateAsync does not provide a way for the caller to get the token that was returned in the REST response.

Its signature is Task AuthenticateAsync(AuthConfig authConfig, CancellationToken cancellationToken = default(CancellationToken)), but it should probably be Task<AuthResponse> AuthenticateAsync(AuthConfig authConfig, CancellationToken cancellationToken = default(CancellationToken)) instead.

Other details:
I'm happy to submit a PR, but what's this project's preferred way to handle changes that would break binary compatibility? Just changing the return type would be breaking. An overload with an out parameter isn't possible because it's an async method. Are you OK with adding an Authenticate2Async method or similar?
@dgvives
Copy link
Contributor

dgvives commented Feb 17, 2021 •
edited
Loading

@Emdot I'm glad you brought this here. Same happens on endpoints returning Task instead of existing Task < DockerApiResponse >

I'm not a maintainer, but I would like to see your approach on this. Regarding breaking binary compatibility, it should get fixed by rebuilding applications with no required changes on code I believe

I encourage you to do it for the sake of having fun

@Emdot
Copy link
Contributor Author

Emdot commented Feb 18, 2021

Unfortunately, I know too many ways to fix API versioning problems, and none of them are ideal. My approach would depend firstly on how willing we are to issue a new major version at this point, secondly on whether we're aiming for binary compatibility vs source compatibility, and thirdly on whether we support the possibility that consumers may have custom implementations of IContainerOperations interface. All of those questions require a maintainer to weigh in. :)

@jterry75
Copy link
Contributor

@Emdot - We have not released a version in a very long time for lots of reasons I'm not getting into haha. Please feel free to overwrite this method and have it be the correct signature that it should have been originally. When @galvesribeiro does a release we will do a major version bump.

@galvesribeiro
Copy link
Member

We can set a release soon with the current codebase if you feel its ok.

If you guys want to fix the method overload as @jterry75 mentions, just feel free to open a PR and we can push the release right after.

@Emdot Emdot linked a pull request Feb 18, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ISystemOperations.AuthenticateAsync now returns AuthResponse.
4 participants
@Emdot @galvesribeiro @jterry75 @dgvives