- Notifications
You must be signed in to change notification settings - Fork673
chore: add authentication type to GitlabAuthenticationError#1793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedJan 2, 2022 • edited by codecovbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by codecovbot
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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.
🚀 New features to boost your workflow:
|
51573dc tof1239fdCompareAdd the type of authentication used to the GitlabAuthenticationErrorexception. Hopefully this will make it easier to help user's debugauthentication issues they run into.
f1239fd tocce2202CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we could add**kwargs toGitlabError and we don't need to reimplement it here? Just pop it from kwargs?
| @@ -722,6 +727,7 @@ def http_request( | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we could actually reuseself.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 withPrivate-Token,Job-Token, andAuthorization (OAuth bearer) from headers. We could probably just pass the keys from the headers dict to not leak stuff accidentally.
| def __str__(self) -> str: | ||
| if self.auth_type: | ||
| return f"{super().__str__()}: authentication_type: {self.auth_type}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we decide to infer the auth type automatically I'd maybe reformulate this a bit. Not sure exactly how, justauthentication_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.
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. |
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.