Add default serveraddress value in remote API /auth
#22254
Conversation
This fix tries to address the issue in moby#22244 where the remote API `/auth` will not set the default value of `serveraddress` if not provided. This behavior happens after only in 1.11.0 and is a regression as in 1.10.3 `serveraddress` will be assigned with `IndexServer` if no value is provided. The default value `IndexServer` is assigned to `serveraddress` if no value provided in this fix. An integration test `TestAuthApi` has been added to cover this change This fix fixes moby#22244. Signed-off-by: Yong Tang <[email protected]>
yongtang
force-pushed
the
22244-remote-api-auth-behavior
branch
from
d959d3c to
9397319
Compare
|
ping @aaronlehmann @kencochrane can you have a look at this? Also if we need this for 1.11.1 |
|
Seems reasonable to me |
|
Can we find the PR that broke the behavior? |
|
Possibly f2d481a, although that included a |
|
Hi @tiborvass it looks like the related commit is in: |
|
@dmcgowan can you please review? |
|
LGTM |
|
@aaronlehmann I don't think we intended to remove this or change to something else, such as |
|
The fix looks good. I'm not sure how I feel about adding a unit test that involves communicating with Docker Hub. We already have some integration tests that do this, and they are inherently flaky because network glitches and hub issues cause failures. I don't know if there are any unit tests that depend on connecitivity to Docker Hub, but if not, adding this one would mean that unit tests can't run offline. Is there a way to cover this in a test without actually communicating with Docker Hub? |
|
Agree with Aaron, unit test should not communicate with hub. It should just check the string. |
|
Sorry, it's morning in my timezone and I didn't realize that this is actually an integration test, not a unit test. So I withdraw my objection. |
|
|
||
| // Test case for #22244 | ||
| func (s *DockerSuite) TestAuthApi(c *check.C) { | ||
| config := types.AuthConfig{ |
There was a problem hiding this comment.
Perhaps add testRequires(c, Network)
There was a problem hiding this comment.
@aaronlehmann would that work for the meantime? ^^
There was a problem hiding this comment.
@thaJeztah: Sounds good.
|
LGTM |
- What I did
This fix tries to address the issue in #22244 where the remote API
/authwill not set the default value ofserveraddressif not provided. This behavior happens only after 1.11.0 and is a regression as in 1.10.3,serveraddresswill be assigned withIndexServerif no value is provided.- How I did it
The default value
IndexServeris assigned toserveraddressif no value provided in this fix.- How to verify it
An integration test
TestAuthApihas been added to cover this change- A picture of a cute animal (not mandatory but encouraged)
This fix fixes #22244.
Signed-off-by: Yong Tang [email protected]