fix(workflows): refactor sample 'workflows_api_quickstart' to comply with the Samples Style Guide and fix tests #13261
Conversation
- Take out unneccesary `os.getenv`s and the return from the sample - As the `__main__` entry point is requested to stay in the script, but not in the sample, it was adjusted accordingly
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 refactors the sample 'workflows_api_quickstart' to comply with the Samples Style Guide. The changes include cleaning up the code inside the region tags, removing unnecessary os.getenvs and returns from the region tags, and adjusting the placement of the __main__ entry point to align with the style guide's recommendations.
Highlights
- Code Cleanup: The code within the region tags has been cleaned up to improve readability and ease of understanding for developers using the quickstart guide.
- Environment Variables: Unnecessary
os.getenvscalls have been removed to simplify the code and reduce dependencies. - main Entry Point: The placement of the
__main__entry point has been adjusted to comply with the Samples Style Guide while still retaining it in the script as requested.
Changelog
- workflows/cloud-client/main.py
- Removed unused imports and environment variable assignments at the beginning of the file.
- Modified the
execute_workflowfunction signature to acceptproject_id,location, andworkflow_idas arguments. - Moved the import statements and TODO comments inside the
execute_workflowfunction and within the region tag. - Updated the
workflows_client.workflow_pathcall to use the new function arguments. - Moved the
return executionstatement inside theelseblock. - Moved the
[END workflows_api_quickstart]tag to after the[END workflows_api_quickstart_execution]tag. - Moved the
__main__block to the end of the file and hardcoded the location and workflow id.
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.
Trivia time!
What is the purpose of region tags in Google Cloud documentation samples?
Click here for the answer
Region tags are used to mark specific sections of code in documentation samples. These tags allow automated systems to extract and test these code snippets, ensuring the samples remain up-to-date and functional.
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. ↩
There was a problem hiding this comment.
Code Review
The pull request aims to refactor the 'workflows_api_quickstart' sample to comply with the Samples Style Guide. The changes involve cleaning up the code inside region tags, removing unnecessary os.getenvs and returns, and adjusting the placement of the __main__ entry point. Overall, the changes improve the readability and maintainability of the sample code.
Summary of Findings
- Hardcoded values: The script uses hardcoded values for
locationandworkflow_idin the__main__block. It's better to use environment variables or command-line arguments to configure these values for flexibility. - Missing error handling: The code lacks explicit error handling for API calls. Adding error handling would make the sample more robust and informative.
Merge Readiness
The pull request is almost ready for merging. Addressing the hardcoded values and adding error handling would further improve the quality of the sample. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
|
Here is the summary of changes. You are about to add 1 region tag.
You are about to delete 1 region tag.
This comment is generated by snippet-bot.
|
|
I'm noticing that the Python 3.9 Test is failing, see log. Possibly due to temporary unavailability of the external API. This will be fixed by adding exponential backoff to the test. The test SHOULD recreate the workflow (to apply the latest changes in the YAML definition), and delete the workflow after it was used. The workflow creation will be fixed and moved to the |
…ogle-cloud-workflows library
…workflow creation
|
@iennae I would like to have feedback from @davidcavazos or @glasnt on this refactor. cc @OremGLG, I'm aware you are working on a related sample for Go, so the learning here could help you. |
| # [END workflows_api_quickstart_execution] | ||
| # [END workflows_api_quickstart] | ||
| return execution | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
As the main entry points have been requested by 'glasnt' to stay in the scripts, but the Style guide suggests not to include them in the sample, it was adjusted accordingly
Can you link to where you're reading this in the style guide?
As you can see in this example, the main method is included within the region tags, so we have an example of how to invoke the function. If the styleguide needs addressing, please raise this out of band.
There was a problem hiding this comment.
I couldn't fit a full explanation in a short sentence, @iennae we could discuss it internally.
- "Methods should avoid a return type whenever possible."
samples-style-guide/#result
Python examples in the Style Guide don't show any return. For example: samples-style-guide/#pattern
Removing returns creates a conflict for samples developers, as returned values help with testing. See a previous review by 'davidcavazos' here.
I've seen a pattern in the repo, which I followed, where the returned object is outside of the sample region tags (so it won't be shown in the documentation).
But if I do that, the function definition shows a type hint of a returned object, when the sample doesn't have any (it should be a None then). What I find that complies with all above is not including the function definition part, as it's in this PR. That is the reason for my decision on the region tags positions.
For instance, my samples in View the access policy of a dataset and view_dataset_access_policy.py#L19
I see 'grayside' has addressed a related matter in Issue #169
- The style guide for Python doesn't show the use of
if __name__ == "__main__":
Only Java and PHP show something similar in Language-specific practices
Please check a previous review by 'davidcavazos' here and samples-style-guide/#no-cli
Review: "Supporting a CLI entry point means additional maintenance."
Guide: "Do not include CLIs for running your sample. [...] Previous practice shows that CLIs are expensive to maintain and detract from the purpose of the snippet."
In addition to external tools and CLIs such as glogin, I understand this part as running the sample from the Terminal by using python3 sample.py as well.
I see this has not been addressed as an Issue in the samples-style-guide repo.
|
PR still in draft, but it seems generally okay. Will wait for PR to come out of draft before detailed review. |
update the copyright year line 31: change "The Google Cloud project id" to "The ID of the Google Cloud project" line 33: add a period line 50: add one more space before the line comment per https://google.github.io/styleguide/pyguide#385-block-and-inline-comments line 51: add a line comment (# For example: myFirstWorkflow)
|
Hi! I have submitted changes requested by Camie, this PR is ready to be reviewed again. I plan to use the approved style for this sample |
Description
Fixes Internal: b/391200147
_idsuffix to project and workflow, to make more explicit what value is expectedos.getenvsand the return from the region tagsconftest.pywhich will be reused for new samples in the same folder__main__entry points have been requested by 'glasnt' to stay in the scripts, but the Style guide suggests not to include them in the sample, it was adjusted accordinglyChecklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)