Update Docker resolver to pass in Authorizer interface #2690
Conversation
|
Something not quite right: |
a59bfc0 to
7913273
Compare
Codecov Report
@@ Coverage Diff @@
## master #2690 +/- ##
==========================================
+ Coverage 43.07% 43.16% +0.08%
==========================================
Files 100 101 +1
Lines 10606 10639 +33
==========================================
+ Hits 4569 4592 +23
- Misses 5316 5323 +7
- Partials 721 724 +3
Continue to review full report at Codecov.
|
remotes/docker/resolver.go
Outdated
| // May return an emptry string or | ||
| // "bearer <some bearer token>" | ||
| // "basic <base64 encoded credentials>" | ||
| Authorize(context.Context, *http.Request) (string, error) |
There was a problem hiding this comment.
Should this just write the header into the request? There may be authorization schemes that add other headers.
There was a problem hiding this comment.
I do prefer that, but I was very hesitant to make argument side effecting part of a public interface. I didn't want to have that argument, but if you also think this case is worth it, I don't mind making that change.
There was a problem hiding this comment.
Updated, better to just keep the interface less opinionated
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
7913273 to
a6198b7
Compare
This creates an authorizer interface which can be shared across multiple requests and more configurable for setting the authorization. This fixes a bug today which doesn't case sharing of tokens between resolve and fetch due to creation of a different base object which caches. Additionally this change moves the implementation closer to being focused on the OCI distribution specification while keeping the Docker specific authorization as a separate component.
An additional change can be done later to remove the credentials function after the callers have updated. There is also an additional change to allow the host/scheme specification to be more robust and have better support for mirrored configuration without needing to create multiple instances for each host. I wanted to keep that separate from the authorization component.
Fixes #1005
Fixes #2683