Fix auth URL #53
Fix auth URL #53
Conversation
The way that the auth url was being used by AuthorizationService was too restrictive and made too many assumptions about the format of the URL; GitLab, for example, uses a different URL format and does not name the service the same as DockerHub does. These changes add a parameter, 'auth_service_name', which allows custom service names that are not simply the same as the registry's host name (e.g. GitlLab uses 'container_registry'), and also changes the URL so that the 'v2/token' location is not necessarily assumed. *This is necessarily a breaking change for existing consumers and will require a majorversion bump*.
|
hello @graingert — would you be able to review and merge in this PR if it's acceptable? |
|
can you add a test case for this change? |
This prevents the access token from being sent by the requests library
Rather than forcing command-line and API consumers to change their code, we keep the old arguments and behavior in, but add a warning, allowing the new behavior to be triggered with new arguments
This puts it in the user's PATH upon package installation
Put errors checking before status checking
|
@graingert thanks for your prompt and helpful review! In my next round of changes, I added backwards-compatible interfaces, both at the API and command-line level, with deprecation warnings; I also added unit tests to check the functionality, and did some other minor cleanup. I'm updating the PR description to reflect that it does a deprecation, rather than a backwards-incompatible change. Currently, the base client integration test fails for me on Please let me know if there are any further changes you'd like me to make for this PR—I'd be happy to discuss. On a related note, I will likely add the capability for the BaseClient to make cross-repository blob mounts in an upcoming PR. It should be straightforward enough, and enables efficiently "moving" Docker images on a registry from one repo to another without downloading, retagging, and uploading the entire image. |
|
@graingert any update on this request? |
|
I don't have permissions to merge PRs |
|
Hello. Since this package appears to be abandoned, I forked it and made several improvements. This includes dropping support for v1 registries, and overhauling how the auth service works. If you're still feeling up to it, I'd be happy to review an updated PR against my fork: |
|
@djmattyg007 sure;seems easy enough; i’ll take a look when I get back to my laptop; if you haven’t heard from me or seen a PR by 9/13, feel free to ping me again. |
|
@myw ping 🙂 |
|
Hi @myw just checking in to ask if you'd had a chance to look at dreg-client yet |
The way that the auth url was being used by AuthorizationService was too restrictive
and made assumptions about the format of the URL that are not part of the standard and are violated by some existing authorization schemes: GitLab's authorization, for example, uses a different URL format (no
v2/tokenpath) and does not name theserviceGET parameter sent to the authorization URL with the same format as DockerHub does (it usescontainer_registry, instead of the registry host name).The changes in this PR do two main things:
auth_service_name, which allows client consumer code to provide custom service names; if not provided, the code defaults to the previous behavior (using the registry hostname).auth_service_url_full. In this parameter, the/v2/tokenpath is no longer automatically assumed on all URLs and must be provided explicitly if needed. This new parameter deprecates the olderauth_service_urlparameter, which will now raise aDeprecationWarningif provided.As a minor additional improvement, the PR also makes the
docker-registry-show.pyscript releasted to users' PATHs upon package installation (via thescriptsargument insetup.py).