Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor 401 retrying into both auth methods #1910

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gmishkin
Copy link

@gmishkin gmishkin commented Oct 28, 2024

See #1904 for details on why.

This change refactors 401 retrying into the common base class, and for token auth the retry is implemented by clearing the session cookie and retrying with the same PAT as recommended.

In a few cases, the code is kind of ugly because in order to implement the retry I need to have a reference to the session object, which is only available if the tokenauth is created through the normal client constructor. If the tokenauth is created directly then the session might not be available. I am not sure if this is a supported case, if it's not then I might be able to clean up the checks for session and the asserts.

Copy link

Label error. Requires exactly 1 of: bug, enhancement, major, minor, patch, skip-changelog. Found:

@gmishkin gmishkin marked this pull request as ready for review October 29, 2024 14:27
@gmishkin gmishkin requested a review from a team as a code owner October 29, 2024 14:27
@gmishkin gmishkin requested a review from ssbarnea October 29, 2024 14:27
@gmishkin gmishkin changed the title refactor 401 retrying into both auth methods DEVPROD-5571: refactor 401 retrying into both auth methods Nov 5, 2024
@gmishkin gmishkin changed the title DEVPROD-5571: refactor 401 retrying into both auth methods refactor 401 retrying into both auth methods Nov 5, 2024
@gmishkin
Copy link
Author

gmishkin commented Nov 8, 2024

@ssbarnea not sure what to do about the check failures, can you advise?

@Re4zOon
Copy link

Re4zOon commented Nov 26, 2024

Hi @adehad!

Can you please review this PR?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants