Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1793 +/- ##
==========================================
+ Coverage 92.02% 92.04% +0.01%
==========================================
Files 76 76
Lines 4790 4801 +11
==========================================
+ Hits 4408 4419 +11
Misses 382 382
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks John! Just a few ideas from my side on this one :) At the beginning I wasn't quite sure about the use case for this, but for authentication errors might be handy with all the job token confusion.
Especially with the job token we could add a link to the endpoints available or something (e.g. have a mapping for the error message).
Sorry, something went wrong.
| def __init__( | ||
| self, | ||
| error_message: Union[str, bytes] = "", | ||
| response_code: Optional[int] = None, | ||
| response_body: Optional[bytes] = None, | ||
| auth_type: str = "", | ||
| ) -> None: | ||
| super().__init__( | ||
| error_message=error_message, | ||
| response_code=response_code, | ||
| response_body=response_body, | ||
| ) | ||
| self.auth_type = auth_type |
There was a problem hiding this comment.
Maybe we could add **kwargs to GitlabError and we don't need to reimplement it here? Just pop it from kwargs?
Sorry, something went wrong.
| response_code=result.status_code, | ||
| error_message=error_message, | ||
| response_body=result.content, | ||
| auth_type=self.auth_type, |
There was a problem hiding this comment.
I think we could actually reuse self.headers and pass it (or just the dict keys) to the exception to infer the auth type, without adding custom variables here.
http_username/http_password is basically dead code as HTTP Basic auth has been out of GitLab since version 10 so we don't need to worry about that (I actually have a local draft that performs password-based OAuth login from that). So we're left with Private-Token, Job-Token, and Authorization (OAuth bearer) from headers. We could probably just pass the keys from the headers dict to not leak stuff accidentally.
Sorry, something went wrong.
|
|
||
| def __str__(self) -> str: | ||
| if self.auth_type: | ||
| return f"{super().__str__()}: authentication_type: {self.auth_type}" |
There was a problem hiding this comment.
If we decide to infer the auth type automatically I'd maybe reformulate this a bit. Not sure exactly how, just authentication_type sounds like it's a variable name defined/passed somewhere. We can maybe check around how more verbose exceptions do it in cpython or some other libraries.
Sorry, something went wrong.
|
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
Sorry, something went wrong.
|
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
Sorry, something went wrong.
|
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
Sorry, something went wrong.
Add the type of authentication used to the GitlabAuthenticationError
exception. Hopefully this will make it easier to help user's debug
authentication issues they run into.