chore(cloudrun): refactor to sample 'cloudrun_service_to_service_receive' and its test #13292
Conversation
… 3.8 and is not required anymore
…Guide. - Apply fixes for Style Guide - Add type hints - Rename variables to be consistent with their values - Add HTTP error codes constants to avoid managing 'magic numbers' - Rewrite comments for accuracy
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @eapl-gemugami, 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!
Summary of Changes
This pull request focuses on refactoring the 'cloudrun_service_to_service_receive' sample and its associated test. The primary goals are to update dependencies, adhere to the latest style guide, improve code clarity with type hints and consistent naming, and enhance documentation. The noxfile configuration was removed as it was outdated. The changes touch both the application code (app.py, receive.py) and the test suite (receive_test.py), aiming for better maintainability and code quality.
Highlights
- Dependency Updates: The pull request updates the dependencies in
requirements.txtandrequirements-test.txtto their latest versions, ensuring compatibility and access to the newest features and security patches. Specifically,pytestwas updated from 8.2.0 to 8.3.5,requestsfrom 2.31.0 to 2.32.3,Flaskfrom 3.0.3 to 3.1.0, andWerkzeugfrom 3.0.3 to 3.1.3. - Code Style and Clarity: The code was refactored to align with the latest style guide, including the addition of type hints to improve readability and maintainability. Variable names were also adjusted for consistency. For example, a type hint was added to the
mainfunction inapp.pyon line 25, and to thereceive_authorized_get_requestfunction inreceive.pyon line 27. - Documentation Improvements: Comments within the code have been rewritten for accuracy and completeness, ensuring that the purpose and functionality of each section are clearly explained. The docstring for
receive_authorized_get_requestinreceive.pywas updated to provide more comprehensive information about the function's return values on lines 35-38. - Test Enhancements: The test suite was updated to include HTTP status code constants for better readability and maintainability. The test function
test_authinreceive_test.pywas renamed totest_authentication_on_cloud_runand updated to use these constants. Theservicesfixture was also refactored and renamed toservice. - Noxfile Configuration Removal: The
noxfile_config.pyfile was removed because it was outdated for Python 3.8 and no longer necessary for the project's configuration.
Changelog
Click here to see the changelog
- run/service-auth/app.py
- Added a return type hint to the
mainfunction.
- Added a return type hint to the
- run/service-auth/noxfile_config.py
- Removed the file as it was outdated and unnecessary.
- run/service-auth/receive.py
- Added a type hint to the
receive_authorized_get_requestfunction. - Rewrote the docstring for
receive_authorized_get_requestfor accuracy and completeness. - Updated the code to use
decoded_tokeninstead ofclaimswhen verifying the token. - Added flask import.
- Added a type hint to the
- run/service-auth/receive_test.py
- Added HTTP status code constants for better readability.
- Refactored the
servicesfixture and renamed it toservice. - Renamed the
test_authfunction totest_authentication_on_cloud_run. - Updated the test assertions to use the new HTTP status code constants.
- Added a comment to indicate that the test raises a CalledProcessError exception for a non-zero exit code.
- Added a comment to indicate that the test cleans up after running.
- Updated the test to use the
PROJECT_IDconstant instead of reading the environment variable directly.
- run/service-auth/requirements-test.txt
- Updated the version of
pytestfrom 8.2.0 to 8.3.5.
- Updated the version of
- run/service-auth/requirements.txt
- Updated the version of
requestsfrom 2.31.0 to 2.32.3. - Updated the version of
Flaskfrom 3.0.3 to 3.1.0. - Updated the version of
Werkzeugfrom 3.0.3 to 3.1.3.
- Updated the version of
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A Cloud Run service,
Secured with token's embrace,
Greets users by name.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
product-auto-label
bot
added
samples
There was a problem hiding this comment.
Code Review
The pull request refactors the 'cloudrun_service_to_service_receive' sample and its test, updating dependencies, removing outdated configurations, applying style guide fixes, adding type hints, renaming variables, adding HTTP error code constants, and rewriting comments. Overall, the changes improve the code's readability, maintainability, and consistency.
Summary of Findings
- Missing docstring: The
servicefixture inreceive_test.pyis missing a docstring, which should be added to explain its purpose. - Inconsistent naming: The
servicesfixture was renamed toservice, but the test function nametest_authentication_on_cloud_runstill refers toservicesin its argument type hint. This should be updated for consistency.
Merge Readiness
The pull request is in good shape and addresses the stated goals. However, addressing the missing docstring and inconsistent naming would further improve the code quality. I am unable to directly approve the pull request, and recommend that others review and approve this code before merging.
…h as this sample is not specific to Cloud Run
snippet-bot
bot
removed
the
snippet-bot:force-run
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
|
(incorrectly marked this PR as fixed by another, please ignore) |
PR Review GoogleCloudPlatform#13292 (review) - Replace HTTP status codes with IntEnum from http.HTTPStatus - Replace hard coded token with a fixture for a fake token - Remove duplicated code for the client, and moving it to a test fixture Also - Add HTTP codes to the app.py `/` endpoint - Replace 'UTF-8' with 'utf-8' to follow the official documentation
|
@glasnt thanks for your review, great insights! |
|
@glasnt I've just committed the approach of using app.py#L31-L37 (flask view outside the sample) was adjusted to support this case. Validating a |
eapl-gemugami
deleted the
paradalicea/fix/cloudrun/service_to_service_receive/b-405225642
branch
Description
Fixes Internal: b/405225642
noxfile_config.pyas it was outdated for Python 3.8 and it isn't necessary anymorehttp.HTTPStatusenumChecklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)