-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
vendor: github.com/docker/docker master (06e3a49d66fa) #5080
Conversation
full diff: moby/moby@v26.1.0...06e3a49 Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5080 +/- ##
==========================================
- Coverage 61.33% 61.31% -0.02%
==========================================
Files 298 295 -3
Lines 20691 20689 -2
==========================================
- Hits 12691 12686 -5
- Misses 7099 7101 +2
- Partials 901 902 +1 |
Signatures of these functions were changed in 80d92fd45007b6395dc2db5f93def3b159dacd7f Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
4a1a6f9
to
1527d62
Compare
select { | ||
case <-ctx.Done(): | ||
return "", ctx.Err() | ||
default: | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to check this only after printing "Please login prior"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigureAuth
does read input from user so thought it was more important to do that before that, as getting the auth config might take some time, so it's more likely to get a context cancellation during that.
We can have it in both places, but didn't want to clutter the code too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, alright yeah I guess that's fine. Might just be a bit weird if the context gets cancelled and then we print "Please login" and then it exits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
full diff: moby/moby@v26.1.0...06e3a49
Plumb context to API callbacks
Signatures of these functions were changed in moby/moby#47536