Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

auth() method should be updated for new API #629

Open
johnflavin opened this issue Feb 16, 2017 · 5 comments
Open

auth() method should be updated for new API #629

johnflavin opened this issue Feb 16, 2017 · 5 comments
Labels
help wanted pinned exempt from being marked as stale question

Comments

@johnflavin
Copy link
Contributor

In docker API versions < 1.23, POSTing registry credentials to /auth would return a 200 if you could log in, and that's all. DockerClient.auth() reflects that, returning only an int.

For API versions >= 1.23, the POST /auth endpoint changed. It now returns a JSON body:

{
     "Status": "Login Succeeded",
     "IdentityToken": "9cbaf023786cd7..."
}

We should update DockerClient.auth() to read this JSON body into a message class. However, I'm not exactly sure how the implementation will work.

Questions:

  • What should we do with the Auth class that gets parsed from the message? We could just return it from the DockerClient.auth() method. But that is maybe sub-optimal because: 1. The "Status" message is redundant with the HTTP status code, at least for now; and 2. The "IdentityToken" is empty for the public Docker Hub, which does not use identity tokens (even though they should). So is it worth returning this almost worthless object?
  • If we don't return the Auth object, what will DockerClient.auth() return? It could still return an int for the status. But that seems sort of dumb to me. At the very least it should be a boolean for whether you are authorized or not.
@mrtnrdl
Copy link

mrtnrdl commented Apr 13, 2017

@johnflavin With a boolean, you can't distinguish a bad parameter from a server error.

Taking that into consideration, I'd go with the int.

@johnflavin
Copy link
Contributor Author

It is true that a boolean return does not tell you which error occurred. But the practice throughout docker-client is that any known error states are thrown as exceptions with specific types corresponding to the error. So you would be able to distinguish the different kinds of error by catching the different exceptions.

Not to say that I agree or disagree with continuing to return an int. Just pointing out that, regardless of what the method ostensibly returns, it can also give information by throwing exceptions.

@mrtnrdl
Copy link

mrtnrdl commented May 5, 2017

After thinking a while about it, I'd go with your suggestion to use the exception mechanism to distinguish the error.

@stale
Copy link

stale bot commented Sep 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 24, 2018
@davidxia davidxia added pinned exempt from being marked as stale and removed stale labels Sep 25, 2018
@dmandalidis
Copy link
Contributor

Hi,

Since this project went on mature status, please re-open this issue (if it still stands) to https://github.com/dmandalidis/docker-client. Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted pinned exempt from being marked as stale question
Projects
None yet
Development

No branches or pull requests

4 participants