Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented Mar 8, 2019

And refactor it to use GetAuthentication, which is just as convenient for callers. This also removes the lookup differences between lookup logic between the two functions (e.g. GetUserLoggedIn used to ignore DockerAuthConfig).

Spun off from #588 here and #596.

@wking wking force-pushed the GetAuthentication-for-logged-in branch from 5a9ae55 to ee8c757 Compare March 8, 2019 20:10
@vrothberg
Copy link
Member

Need to update containers/image to reflect the latest changes in containers/storage:

# github.com/containers/image/storage
storage/storage_image.go:735:55: not enough arguments in call to s.imageRef.transport.store.SetImageBigData
	have (string, string, []byte)
	want (string, string, []byte, func([]byte) (digest.Digest, error))

Code LGTM 👍 Thanks, @wking !

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 11, 2019

If we are doing this, I’d just remove the method; a logrus.Warn is not really wanted by the callers and it’s not actionably by end users.

I do need to check the earlier history of the code (notably #333 ), the different handling and/or the .docker fallback mentioned in #596 might actually have been intentional — I can’t remember any more.

@wking wking force-pushed the GetAuthentication-for-logged-in branch from ee8c757 to 2ec5f3f Compare March 18, 2019 14:16
Use GetAuthentication directly, replacing:

  username, err := GetUserLoggedIn(sys, registry)

with:

  username, _, err := GetAuthentication(sys, registry)

This is just as convenient and removes the differences between lookup
logic from the two functions (e.g. GetUserLoggedIn ignored
DockerAuthConfig).  I've removed the function instead of deprecating
at Miloslav's request [1].

GetUserLoggedIn originally landed in f28367e (Add docker/config
package to containers/image/pkg, 2017-08-29, containers#333), and neither the
commit message nor the pull request motivate the diversion from the
GetAuthentication lookup logic.

[1]: containers#597 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the GetAuthentication-for-logged-in branch from 2ec5f3f to e64a79b Compare March 18, 2019 14:17
@wking
Copy link
Contributor Author

wking commented Mar 18, 2019

If we are doing this, I’d just remove the method...

I've pushed ee8c757 -> e64a79b, removing it.

I do need to check the earlier history of the code (notably #333 ), the different handling and/or the .docker fallback mentioned in #596 might actually have been intentional...

I checked, and saw no discussion in either the commit message or pull request that motivated the different lookup logic. You can obviously still check for yourself too ;).

@vrothberg
Copy link
Member

@wking would you rebase, so we can merge it? Note that podman-login uses GetUserLoggedIn() and must be updated accordingly when we vendor containers/image over there (Cc @baude @mheon).

@rhatdan
Copy link
Member

rhatdan commented Apr 3, 2019

@wking ping...

@vrothberg
Copy link
Member

I am updating this PR, so we can merge it.

@vrothberg
Copy link
Member

Note to ourselves, we must update Podman (and Buildah?) afterward.

@vrothberg
Copy link
Member

Fixes: #596

@vrothberg
Copy link
Member

LGTM, @mtrmac @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Apr 12, 2019

LGTM

@rhatdan rhatdan merged commit 37bab02 into containers:master Apr 12, 2019
@wking wking deleted the GetAuthentication-for-logged-in branch April 25, 2019 19:30
@lio24
Copy link

lio24 commented Mar 11, 2020

I need window 10 to download docker on my PC

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.

5 participants