core: add missing prefix property to auth backend #165
Conversation
ref oras-project#164 (comment) Signed-off-by: tarilabs <matteo.mortari@gmail.com>
oras/auth/__init__.py
Outdated
| @@ -14,10 +14,11 @@ class AuthenticationException(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| def get_auth_backend(name="token", session=None, **kwargs): | |||
| def get_auth_backend(name="token", session=None, prefix="https", **kwargs): | |||
There was a problem hiding this comment.
@tarilabs this won't always be https - specifically in testing. I think we'd want to inherit this from the parent that created it, and use the same convention there with insecure as a boolean to determine the prefix (and not hard code http or https). Let me know your thoughts.
There was a problem hiding this comment.
this line is specifically about the default isn't it,
and is mainly never used because here:
is set explicitly.
am I missing something? 🤔
There was a problem hiding this comment.
Yes I think you want to set it based on the insecure parameter:
self.prefix: str = "http" if insecure else "https"
Which is in the init of the provider.
There was a problem hiding this comment.
mmm, I've actually mimic-ed on existing code and did that in line 23 of this file.
making the line you quote:
another (unused) default.
I'm happy to refactor this code differently if that helps, I was just trying to go with the existing "code convention"? :)
There was a problem hiding this comment.
hope the comment in https://github.com/oras-project/oras-py/pull/165/files#r1805164912 answer the last question, but if that helps, here is the run-down of how keeping the same existing code-style as present today on main, the prefix is set for the auth backed--see below steps with my reasoning out loud, and happy to be corrected!
If you want me to refactor differently, kindly let me know in which way differing from current style; thanks!
oras/provider.py
Outdated
| @@ -78,7 +78,7 @@ def __init__( | |||
| self.session.cookies.set_policy(DefaultCookiePolicy(allowed_domains=[])) | |||
|
|
|||
| # Get custom backend, pass on session to share | |||
| self.auth = oras.auth.get_auth_backend(auth_backend, self.session) | |||
| self.auth = oras.auth.get_auth_backend(auth_backend, self.session, self.prefix) | |||
There was a problem hiding this comment.
step 1.
the prefix of provider is set a few lines above:
Line 69 in 36ef98a
and is "pass forward" as suggested to the call of oras.auth.get_auth_backend()
oras/auth/__init__.py
Outdated
| @@ -14,10 +14,11 @@ class AuthenticationException(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| def get_auth_backend(name="token", session=None, **kwargs): | |||
| def get_auth_backend(name="token", session=None, prefix="https", **kwargs): | |||
There was a problem hiding this comment.
step 2.
unless otherwise specified, this function get_auth_backend() parameter prefix defaults to https; but for all usages in oras code, is set from provider's prefix, as we seen in step1.
oras/auth/__init__.py
Outdated
| backend = auth_backends.get(name) | ||
| if not backend: | ||
| raise ValueError(f"Authentication backend {backend} is not known.") | ||
| backend = backend(**kwargs) | ||
| backend.session = session or requests.Session() | ||
| backend.prefix = prefix |
There was a problem hiding this comment.
step 3.
the determined backend (either token or basic) get .prefix property set to the value received in step 2
this maintains code-style for .session another property set on determined backend.
| @@ -19,6 +19,7 @@ class AuthBackend: | |||
|
|
|||
| def __init__(self, *args, **kwargs): | |||
| self._auths: dict = {} | |||
| self.prefix: str = "https" | |||
There was a problem hiding this comment.
step 4.
the base AuthBackend by default would have a property prefix valorized to https, but as we just seen in step 3, this is set to the actual prefix value.
|
@tarilabs I understand your logic - the question is if a user is ever going to call get_auth_backend(name="token", session=None, prefix="bananas")In which case my preference is to better control that, and (although there is redundancy in setting the prefix) you can better control that with: get_auth_backend(name="token", session=None, insecure=False):
...
self.prefix = "http" if insecure else "https"My preference is to generally not have open string arguments like that, but instead booleans that control them. |
|
thank you for taking the time to explain to me this "gap" @vsoch ; I believe I got it now; let me refactor it 👍 |
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
|
Refactored and maintained version update and changelog, so to make a release to support conversations in #164 |
ref #164 (comment)