Fix login/logout for docker.io/vendor #763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 11, 2021
mtrmac
force-pushed
the
docker.io-vendor
branch
from
67bae5d to
d7b2c92
Compare
mtrmac
force-pushed
the
docker.io-vendor
branch
from
d7b2c92 to
d1f0293
Compare
Relying on login/logout tests in Podman , running in containers/podman#11538, for at least a smoke-test. |
mtrmac
force-pushed
the
docker.io-vendor
branch
from
d1f0293 to
49c1707
Compare
Signed-off-by: Sascha Grunert <[email protected]> Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Make the test fully table-driven now. Signed-off-by: Miloslav Trmač <[email protected]>
The caller shouldn't care anyway. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Only test opts.All once instead of three times. Don't even start worrying about key/registry with --all. Signed-off-by: Miloslav Trmač <[email protected]>
This avoids duplication and non-obvious interactions (where "!= 0" means "== 1"), and is more similar to Login. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This allows looking up namespace credentials by GetCredentials, and fixes GetAllCredentials to normalize the returned keys. Signed-off-by: Miloslav Trmač <[email protected]>
We can now call GetCredentials with namespaced keys, so simplify. The ref value is still computed, we'll remove that momentarily. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... and instead primarily use the string key. This allows using a docker.io/vendor namespace. Also rename parseRegistryArgument to parseCredentialsKey, the argument is not just a registry. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
docker.io-vendor
branch
from
49c1707 to
f083242
Compare
|
The c/image infrastructure was merged, so this is ready for review. |
|
/approve |
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, rhatdan, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative to #757
, depends on UNMERGED containers/image#1373.Now that
GetCredentialsallows looking up namespaced credentials, use that instead ofGetCredentialsForRef, which avoids thedocker.io/vendor→docker.io/library/vendornormalization issue; use reference parsing only to validate the syntax.Also refactor a bit of the argument length handling and
logout --all.See individual commit messages for details.
Warning: Only unit-tested, and the actual functionality does not have test coverage. I didn’t actually try this in practice.