Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented Mar 9, 2019

There have been redundant calls here since two ref.ref.Hostname() calls were added in aaedc64 (#52). At that point the two calls were separated by a dockerHostname check which could have been shifted by two lines to avoid the doubled function calls. But in f28367e (#333) the dockerHostname check moved to a separate function entirely (newDockerClientWithDetails) while the Domain() calls remained together in newDockerClientFromRef. So now there is no longer any reason for the second call, and this pull request drops it.

@vrothberg
Copy link
Member

Good catch, @wking!
LGTM

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 11, 2019

LGTM pending tests. Thanks!

There have been redundant calls here since two ref.ref.Hostname()
calls were added in aaedc64 (Implement lookaside storage for
signatures for Docker registries, 2016-08-11, containers#52).  At that point the
two calls were separated by a dockerHostname check which could have
been shifted by two lines to avoid the doubled function calls.  But in
f28367e (Add docker/config package to containers/image/pkg,
2017-08-29, containers#333) the dockerHostname check moved to a separate
function entirely (newDockerClientWithDetails) while the Domain()
calls remained together in newDockerClientFromRef.  So now there is no
longer any reason for the second call, and this commit drops it.

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the drop-redundant-domain-call branch from 5683b91 to 9e941c2 Compare March 18, 2019 14:21
@wking
Copy link
Contributor Author

wking commented Mar 18, 2019

Heh, Travis couldn't find the build :p. Rebased onto master with 5683b91 -> 9e941c2 to trigger another round of tests.

@mtrmac mtrmac merged commit 17d76e8 into containers:master Mar 18, 2019
@wking wking deleted the drop-redundant-domain-call branch April 25, 2019 19:30
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.

3 participants