docs: sample migration deduplication #13804
Conversation
product-auto-label
bot
added
samples
Summary of ChangesHello @chalmerlowe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring and cleanup of Google Cloud authentication sample code. It eliminates redundant Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly deduplicates the authentication samples by removing the older snippets.py and its related files, and migrating the newer samples from a temporary directory. The changes look good overall.
I've left a few comments:
- In
requirements.txt, thepytestdependency should be moved to a separate test-specific requirements file to avoid installing it in production. Also, the version specifiers forpytestneed to be adjusted to cover all supported Python versions, and the===operator should be used for version pinning as per repository guidelines. - In
snippets_test.py, two tests rely on theGOOGLE_APPLICATION_CREDENTIALSenvironment variable. I've suggested adding logic to skip these tests if the variable is not set, which will make the test suite more robust.
| pytest===8.4.2; python_version == '3.9' | ||
| pytest==9.0.2; python_version > '3.9' |
There was a problem hiding this comment.
There are a few issues with how pytest is specified here:
-
Test dependencies in production requirements:
pytestis a testing dependency and should not be included in the mainrequirements.txtfile. This will cause it to be installed in production environments where it's not needed. It's standard practice to keep test dependencies in a separate file, likerequirements-test.txtordev-requirements.txt. -
Incomplete Python version coverage: The
pytestdependency is only specified for Python 3.9 and greater. The project's previous test configuration (noxfile.py) supported Python 3.7 and 3.8. If these versions are still supported, tests will fail becausepytestwon't be installed. Please ensure test dependencies are available for all supported Python versions. -
Version specifier: For version pinning in
requirements.txt, the===operator is preferred to prevent automated updates from overwriting specific version numbers required for certain Python runtimes.
I recommend removing these lines from this file and adding the corrected pytest dependency to a dedicated test requirements file, ensuring === is used for version pinning.
References
- Use the
===operator inrequirements.txtfor version pinning to prevent automated updates from overwriting specific version numbers required for certain Python runtimes.
|
|
||
| with credentials_patch: | ||
| snippets.explicit_compute_engine(project) | ||
| def test_idtoken_from_service_account(capsys: CaptureFixture) -> None: |
There was a problem hiding this comment.
This test will fail if the GOOGLE_APPLICATION_CREDENTIALS environment variable is not set, as SERVICE_ACCOUNT_FILE will be None. It's good practice to skip tests that depend on environment variables when they aren't available. You can use the pytest.mark.skipif decorator for this.
Example:
import pytest
# ...
@pytest.mark.skipif(not SERVICE_ACCOUNT_FILE, reason="GOOGLE_APPLICATION_CREDENTIALS not set")
def test_idtoken_from_service_account(capsys: CaptureFixture) -> None:
# ... test implementationThis will make your test suite more robust in different environments.
| scope = "https://www.googleapis.com/auth/cloud-platform" | ||
| snippets.accesstoken_from_impersonated_credentials( | ||
| impersonated_service_account, scope | ||
| def test_verify_google_idtoken() -> None: |
There was a problem hiding this comment.
Similar to test_idtoken_from_service_account, this test depends on the GOOGLE_APPLICATION_CREDENTIALS environment variable and will fail if it's not set. This test should also be skipped when the variable is missing to prevent unexpected test failures.
Example:
import pytest
# ...
@pytest.mark.skipif(not SERVICE_ACCOUNT_FILE, reason="GOOGLE_APPLICATION_CREDENTIALS not set")
def test_verify_google_idtoken() -> None:
# ... test implementation1 participant
Deleting older
snippets.pyfiles because the samples in it are adequately covered by the samples in these other files that were recently imported fromgoogle-auth-library-python:What do we need to do to make this happen and have it be successful?
[START auth_cloud_implicit]is replaced by the sample in this file:authenticate_implicit_with_adc.py >
[START auth_cloud_implicit_adc][START auth_cloud_explicit]is replaced by the sample in this file:authenticate_explicit_with_adc.py >
[START auth_cloud_explicit_adc][START auth_cloud_explicit_compute_engine]is replaced by the sample in this file:idtoken_from_metadata_server.py >
[START auth_cloud_idtoken_metadata_server][START auth_cloud_accesstoken_impersonated_credentials]is replaced by the sample in this file:idtoken_from_impersonated_credentials.py >
[START auth_cloud_idtoken_impersonated_credentials]