-
Notifications
You must be signed in to change notification settings - Fork 5
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: Add support for customer compute service account (#51, @hnawar) #52
Conversation
@vsoch @johanneskoester I am supporting a snakemake user to migrate to Batch, one of the requirement that came up was the use of custom service account. This is my attempt to add support for it, I would appreciate your guidance on how best to contribute and test this PR --------- Signed-off-by: Hatem Nawar <[email protected]> Co-authored-by: hatem.nawar <[email protected]> Co-authored-by: Johannes Köster <[email protected]>
Warning Rate limit exceeded@johanneskoester has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the Snakemake executor plugin for Google Cloud Batch by adding support for custom service accounts. Users can now specify a service account email in the documentation, which is integrated into the executor's settings and job allocation policies, improving flexibility and security in managing cloud resources. Additionally, the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
pyproject.toml
is excluded by!pyproject.toml
Files selected for processing (4)
- .gitignore (1 hunks)
- docs/further.md (1 hunks)
- snakemake_executor_plugin_googlebatch/init.py (1 hunks)
- snakemake_executor_plugin_googlebatch/executor.py (2 hunks)
Additional context used
Path-based instructions (2)
snakemake_executor_plugin_googlebatch/__init__.py (1)
Pattern
**/*.py
: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.snakemake_executor_plugin_googlebatch/executor.py (1)
Pattern
**/*.py
: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Markdownlint
docs/further.md
249-249: Column: 1
Hard tabs(MD010, no-hard-tabs)
250-250: Column: 1
Hard tabs(MD010, no-hard-tabs)
251-251: Column: 1
Hard tabs(MD010, no-hard-tabs)
252-252: Column: 1
Hard tabs(MD010, no-hard-tabs)
253-253: Column: 1
Hard tabs(MD010, no-hard-tabs)
Ruff
snakemake_executor_plugin_googlebatch/executor.py
388-388: Missing return type annotation for public function
get_service_account
(ANN201)
388-388: Missing type annotation for
self
in method(ANN101)
388-388: Missing type annotation for function argument
job
(ANN001)
Additional comments not posted (4)
.gitignore (1)
4-4
: Change Approved: Addedpoetry.lock
to.gitignore
.The addition of
poetry.lock
to the.gitignore
file is a sensible choice if the intention is to manage dependencies without tracking the lock file in version control.snakemake_executor_plugin_googlebatch/__init__.py (1)
158-165
: Change Approved: Addedservice_account
attribute toExecutorSettings
.The addition of the
service_account
attribute enhances the configurability of theExecutorSettings
class, allowing users to specify a service account email. The metadata is correctly set to indicate its optional nature and environment variable support.docs/further.md (1)
243-255
: Change Approved: Addedgooglebatch_service_account
parameter.The addition of the
googlebatch_service_account
parameter in thehello_world
rule allows users to specify a custom compute service account email, enhancing flexibility and customization.Tools
Markdownlint
249-249: Column: 1
Hard tabs(MD010, no-hard-tabs)
250-250: Column: 1
Hard tabs(MD010, no-hard-tabs)
251-251: Column: 1
Hard tabs(MD010, no-hard-tabs)
252-252: Column: 1
Hard tabs(MD010, no-hard-tabs)
253-253: Column: 1
Hard tabs(MD010, no-hard-tabs)
snakemake_executor_plugin_googlebatch/executor.py (1)
362-367
: Verify the integration of service account logic.Ensure that the
get_service_account
method is correctly retrieving and assigning the service account to theallocation_policy
. Verify that this change aligns with the intended functionality and does not introduce any unintended side effects.Verification successful
Integration of service account logic is correct.
The
get_service_account
method is properly integrated into the code. It retrieves the service account and assigns it to theallocation_policy
as intended. No issues were found with this integration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `get_service_account` method. # Test: Search for the usage of `get_service_account` to ensure it's correctly integrated. rg --type python -A 5 $'get_service_account'Length of output: 74
Script:
#!/bin/bash # Description: Verify the integration of the `get_service_account` method. # Test: Search for the usage of `get_service_account` to ensure it's correctly integrated. rg --type py -A 5 $'get_service_account'Length of output: 1076
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- tests/init.py (2 hunks)
Additional context used
Path-based instructions (1)
tests/__init__.py (1)
Pattern
**/*.py
: Do not suggest to add trailing commas.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Additional comments not posted (2)
tests/__init__.py (2)
32-32
: LGTM! Ensure consistency with the updated type annotation.The return type annotation change to
snakemake.settings.types.RemoteExecutionSettings
appears correct given the import change.
4-4
: Verify the import path forsnakemake.settings.types
.Ensure that
snakemake.settings.types
is the correct module path forRemoteExecutionSettings
. This change suggests a restructuring in the Snakemake library.
🤖 I have created a release *beep* *boop* --- ## [0.5.0](v0.4.0...v0.5.0) (2024-08-14) ### Features * Add support for customer compute service account ([#51](#51), [@hnawar](https://github.com/hnawar)) ([#52](#52)) ([8678e20](8678e20)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Test for PR #51.
Summary by CodeRabbit
New Features
hello_world
rule, enhancing flexibility for cloud interactions.Bug Fixes
.gitignore
to exclude thepoetry.lock
file, ensuring cleaner version control.Documentation
hello_world
rule to include the newgooglebatch_service_account
parameter, improving user guidance.