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

feat: implement base classes for credentials and request sessions #1551

Merged
merged 18 commits into from
Jul 16, 2024

Conversation

ohmayr
Copy link
Contributor

@ohmayr ohmayr commented Jul 5, 2024

This PR implements base classes for credentials and request sessions to capture the common logic between synchronous and asynchronous code.

@ohmayr ohmayr changed the title implement base classes for credentials and request sessions feat: implement base classes for credentials and request sessions Jul 5, 2024
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@ohmayr ohmayr marked this pull request as ready for review July 5, 2024 19:45
@ohmayr ohmayr requested review from a team as code owners July 5, 2024 19:45
@ohmayr ohmayr added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 5, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 5, 2024
@ohmayr ohmayr force-pushed the implement-base-requests-and-credentials branch from 6d0d1a4 to c22119b Compare July 5, 2024 20:18
Copy link

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but get sign-off from auth team.

Quick observation: When refactoring file contents into a new file but still keeping the old file, it is helpful to reviewers if you can make the new file share the old file's previous history, so reviewers can see the diff's between the old file and the new as well as between the the old file's previous version and new version. Instructions are here: https://stackoverflow.com/a/44036771/23828460 (also see https://www.mankier.com/1/git-cp#Synopsis)

No need to do it now after the fact, but keep it in mind for future refactorings.

google/auth/transport/_requests_base.py Outdated Show resolved Hide resolved
headers["authorization"] = "Bearer {}".format(
_helpers.from_bytes(token or self.token)
)
"""Trust boundary value will be a cached value from global lookup.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this was in the original file, but it's weird having a string floating as a comment rather than a proper Python comment.

google/auth/_credentials_base.py Outdated Show resolved Hide resolved
_DEFAULT_TIMEOUT = 120 # in second


class _BaseAuthorizedSession(metaclass=abc.ABCMeta):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts are similar here re:codesharing. Since the async code is a "colored" function I think it's safe to assume that the sync code and async code should never be in the same callstack.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of the async semantics, yes that is a valid consideration. But there are still common parts of the logic are re-used between sync and async and those would make sense to consider factoring out. (I think in the rant you linked to, that corresponds to the "blue" functions.)

@parthea
Copy link
Contributor

parthea commented Jul 15, 2024

nox > flake8 --import-order-style=google --application-import-names=google,tests,system_tests google tests tests_async
google/auth/credentials.py:26:1: I100 Import statements are in the wrong order. 'from google.auth._credentials_base import _BaseCredentials' should be before 'from google.auth._refresh_worker import RefreshThreadManager'
google/auth/transport/requests.py:41:1: I100 Import statements are in the wrong order. 'import google.auth.transport._mtls_helper' should be before 'from google.auth.transport._requests_base import _BaseAuthorizedSession'
nox > Command flake8 --import-order-style=google --application-import-names=google,tests,system_tests google tests tests_async failed with exit code 1
nox > Session lint failed.

@ohmayr ohmayr merged commit 036dac4 into main Jul 16, 2024
16 checks passed
@ohmayr ohmayr deleted the implement-base-requests-and-credentials branch July 16, 2024 15:22
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.

5 participants