Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented Feb 25, 2019

Shifted a number of auth-getting unit tests from docker_client_test.go into config_test.go, since they only excercise config.go logic. They'd been in their previous location since landing in d30079f (#96).

Spun off from #588.

@wking
Copy link
Contributor Author

wking commented Feb 25, 2019

GitHub doesn't do a great job of showing these moves, but this is basically a giant copy/paste with some config.GetAuthentication -> GetAuthentication changes.

$ git describe
v1.3-49-gd8920c2
$ git blame -C pkg/docker/config/config_test.go | grep Trevor
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800   4) 	"encoding/base64"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800   5) 	"encoding/json"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800   9) 	"path/filepath"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800  10) 	"reflect"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800  14) 	"github.com/containers/storage/pkg/homedir"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800  15) 	"github.com/pkg/errors"
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 236) 			username, password, err := GetAuthentication(sys, tc.hostname)
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 319) 		username, password, err := GetAuthentication(nil, tc.hostname)
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 390) 	username, password, err := GetAuthentication(nil, "index.docker.io")
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 443) 	username, password, err := GetAuthentication(nil, "index.docker.io")
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 454) 	username, password, err = GetAuthentication(nil, "index.docker.io")
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 465) 	username, password, err = GetAuthentication(nil, "index.docker.io")
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 477) 	username, password, err = GetAuthentication(nil, "index.docker.io")
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 484) }
d8920c26 pkg/docker/config/config_test.go (W. Trevor King  2019-02-25 14:29:56 -0800 485) 

@wking
Copy link
Contributor Author

wking commented Feb 25, 2019

I could simplify further by dropping the test-only types (e.g. testAuthConfigEntry is the same as dockerAuthConfig). I haven't done that yet to keep the change easy to review, but I can do it in this commit, or in a separate commit in this PR, or in a follow-up PR. Just let me know if/what you want :).

@wking wking force-pushed the move-docker-config-tests branch from d8920c2 to 79cc881 Compare February 25, 2019 22:43
Shifted a number of auth-getting unit tests from docker_client_test.go
into config_test.go, since they only excercise config.go logic.
They'd been in their previous location since landing in d30079f (Be
benevolent to .docker/config.json file, 2016-09-20, containers#96).

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the move-docker-config-tests branch from 79cc881 to 5299d35 Compare February 27, 2019 22:39
@wking
Copy link
Contributor Author

wking commented Feb 27, 2019

Rebased onto master with 79cc881 -> 5299d35 to address GitHub's "This branch is out-of-date with the base branch" message.

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 1, 2019

LGTM. Thanks!

I could simplify further by dropping the test-only types (e.g. testAuthConfigEntry is the same as dockerAuthConfig).

That would defeat the point of testing that the file decoding works as expected, and that the decoded types were not changed without anyone noticing. Ideally we should have hard-coded fixtures in the repo instead of creating the configs at runtime.

@mtrmac mtrmac merged commit a4104e0 into containers:master Mar 1, 2019
@wking wking deleted the move-docker-config-tests branch March 1, 2019 06:38
@wking
Copy link
Contributor Author

wking commented Mar 1, 2019

Ideally we should have hard-coded fixtures in the repo...

Filed with #593 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants