The Wayback Machine - https://web.archive.org/web/20260310060112/https://github.com/docker/cli/pull/546
Skip to content

Set APIVersion on the client, even when Ping fails#546

Merged
cpuguy83 merged 1 commit intodocker:masterfrom
dnephin:fix-version-on-failure
Sep 23, 2017
Merged

Set APIVersion on the client, even when Ping fails#546
cpuguy83 merged 1 commit intodocker:masterfrom
dnephin:fix-version-on-failure

Conversation

@dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 20, 2017

Fixes #149

Refactor to support testing
Also add tests

@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #546 into master will increase coverage by 0.14%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   49.08%   49.23%   +0.14%     
==========================================
  Files         200      200              
  Lines       16447    16453       +6     
==========================================
+ Hits         8073     8100      +27     
+ Misses       7954     7933      -21     
  Partials      420      420
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐄 Just one question 👼

t.Run(testcase.doc, func(t *testing.T) {
passRetriever := func(_, _ string, _ bool, attempts int) (passphrase string, giveup bool, err error) {
switch attempts {
case 0, 1, 2:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand why those are here — well, it's for notary.passRetriever I guess, but those magic number do not help me why we should fail the 3 first time 😅

Copy link
Contributor Author

@dnephin dnephin Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really no significance, it could also be just 0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Refactor to support testing
Also add tests

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit a41caad into docker:master Sep 23, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.10.0 milestone Sep 23, 2017
@dnephin dnephin deleted the fix-version-on-failure branch September 23, 2017 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment