Proposed fix for ACR caching while providing guidance for GitLab users. #1628
Conversation
…or private / authenticated ones (not applicable to public CRs).
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
for more information, see https://pre-commit.ci
…eter if "token URL is known".
…com/wallyhall/binderhub into bugfix/1620-restore-acr-caching-fix
|
Has anyone advice for testing this PR? The updated tests are largely all integration ... and I'm not entirely clear on the consequences of what I've changed with regards to other container registries (and I think that probably is why the original PR this seeks to "patch" probably wasn't caught for the ACR scenario I'm trying to repair here). I've added an attempt at a regression test too, but I'm no Python wizard and I've been unable to get the tests running locally (I've relied on CI). |
|
Can any maintainer review/merge this? We've stuck with the same problem using ACR. Thanks! |
There was a problem hiding this comment.
Sorry for the delay in reviewing. Over time the registry class has been incrementally modified to support other registries but unfortunately the documentation of how each registry works hasn't always been included, hence the difficulty in maintaining this code.
I think the best we can do is to ensure the existing tests pass unmodified (unless it's clear why the test needs to be changed), and to add more tests for new functionality or fixes as you've done.
| "service": "service.1", | ||
| "scope": "scope.2", | ||
| }, | ||
| ), |
| @@ -196,6 +204,49 @@ async def test_get_token(): | |||
| assert token == test_handle["token"] | |||
|
|
|||
|
|
|||
| async def test_get_token_with_config_supplied_service(): | |||
There was a problem hiding this comment.
It looks like this is a copy of test_get_token apart from the arguments to registry._get_token(). Can you use @pytest.mark.parametrize instead to reduce duplication?
| @@ -230,7 +281,7 @@ async def test_get_image_manifest(tmpdir, token_url_known): | |||
| f, | |||
| ) | |||
| if token_url_known: | |||
| token_url = url + "/token" | |||
| token_url = url + "/token?service=container_registry" | |||
There was a problem hiding this comment.
Is this needed? If it's testing another scenario can you use @pytest.mark.parametrize to test with and without this extra parameter?
|
I have setup BinderHub with ACR and in principal it works until the built image is downloaded where it fails with: I guess it is because of the issue described in #1620. Is there a way to test this patch in my instance? I am a k8s / helm noobie so it would awesome could point out the required steps to test this, thanks a lot! |
Proposed fix for #1620
This supersedes a single line change of PR #1283 ... and provides documentation (like already exists for ACR) to maintain GitLab compatibility (but the documented instructions have not been validated).