fix: properly catch and report ApiExceptions #42
Conversation
📝 WalkthroughWalkthroughThe changes introduce an additional error handling mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Retry as _kubernetes_retry
participant Kubernetes as Kubernetes API
participant Auth as Authentication
Caller->>Retry: Call _kubernetes_retry
Retry->>Kubernetes: Execute request
Kubernetes-->>Retry: Throw ApiException(e)
Retry->>Retry: Check e.status
alt e.status == 401
Retry->>Auth: Reauthenticate
Auth-->>Retry: Credentials refreshed
Retry->>Kubernetes: Retry request
else Other error
Retry->>Caller: Raise WorkflowError("Kubernetes request failed: " + e)
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_kubernetes/__init__.py (1)
578-583: Improve exception chaining for better error traceability.The addition of error handling for non-401 API exceptions is good, but it should use exception chaining to preserve the original exception's traceback, which is helpful for debugging.
except kubernetes.client.rest.ApiException as e: if e.status == 401: # Unauthorized. # Reload config in order to ensure token is # refreshed. Then try again. return self._reauthenticate_and_retry(func) - raise WorkflowError("Kubernetes request failed.", e) + raise WorkflowError("Kubernetes request failed.", e) from eThis change ensures that the original
ApiExceptiontraceback is preserved when theWorkflowErroris raised, making debugging easier.🧰 Tools
🪛 Ruff (0.8.2)
583-583: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_kubernetes/__init__.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
snakemake_executor_plugin_kubernetes/__init__.py
🪛 Ruff (0.8.2)
snakemake_executor_plugin_kubernetes/__init__.py
583-583: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: testing
- GitHub Check: linting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
69-74: Review: Secure the MinIO Setup Credentials
The "Setup minio" step is correctly configured to initialize a MinIO server. However, using hard-coded credentials (access_key: minioandsecret_key: minio123) might be acceptable for testing but can lead to inadvertent exposure if not handled correctly. Consider using GitHub secrets to store these credentials for improved security and maintainability.
76-82: Review: Validate MinIO Test Step Configuration
The "Test minio" step exports the required AWS environment variables and attempts to create an S3 bucket namedteston the local MinIO endpoint. Ensure that the AWS CLI is available in the runner environment and that the endpoint URL and bucket creation command behave as expected. Optionally, you could add a verification step to check if the bucket was successfully created, which would provide more robust feedback in the CI process.
83-111: General CI Workflow Integration Note
The overall structure of the CI workflow remains clear and the new MinIO steps are integrated appropriately within the testing job. Ensure that these changes do not interfere with or mask issues related to the ApiException error handling in your Kubernetes retry logic, as mentioned in the PR objectives. It might be helpful to include integration tests that validate both the Kubernetes error reporting and the MinIO setup if they interact in any way.🧰 Tools
🪛 actionlint (1.7.4)
83-83: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
93-93: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/assets/minio.yaml(1 hunks).github/workflows/ci.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/assets/minio.yaml
2-2: unexpected key "apiVersion" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
2-2: "on" section is missing in workflow
(syntax-check)
2-2: "jobs" section is missing in workflow
(syntax-check)
3-3: unexpected key "kind" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
4-4: unexpected key "metadata" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
6-6: unexpected key "spec" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🪛 YAMLlint (1.35.1)
.github/workflows/assets/minio.yaml
[warning] 27-27: wrong indentation: expected 8 but found 6
(indentation)
[warning] 31-31: wrong indentation: expected 8 but found 6
(indentation)
[warning] 33-33: wrong indentation: expected 10 but found 8
(indentation)
[error] 33-33: trailing spaces
(trailing-spaces)
[warning] 37-37: wrong indentation: expected 10 but found 8
(indentation)
[warning] 40-40: wrong indentation: expected 10 but found 8
(indentation)
[warning] 45-45: wrong indentation: expected 10 but found 8
(indentation)
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: CI
.github/workflows/assets/minio.yaml
[error] 1-1: Error: resource mapping not found for name: 'my-minio-fs' namespace: '' from '.github/workflows/assets/minio.yaml': no matches for kind 'Deployment' in version 'extensions/v1'. Ensure CRDs are installed first.
🔇 Additional comments (1)
.github/workflows/assets/minio.yaml (1)
1-12: PVC Block Looks GoodThe PersistentVolumeClaim is defined correctly with API version
v1and appropriate storage request.🧰 Tools
🪛 actionlint (1.7.4)
2-2: unexpected key "apiVersion" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
2-2: "on" section is missing in workflow
(syntax-check)
2-2: "jobs" section is missing in workflow
(syntax-check)
3-3: unexpected key "kind" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
4-4: unexpected key "metadata" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
6-6: unexpected key "spec" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🪛 GitHub Actions: CI
[error] 1-1: Error: resource mapping not found for name: 'my-minio-fs' namespace: '' from '.github/workflows/assets/minio.yaml': no matches for kind 'Deployment' in version 'extensions/v1'. Ensure CRDs are installed first.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/assets/minio.yaml (4)
1-25: Deployment Resource: Validate Configuration and Best Practices
The Deployment resource is correctly updated to useapiVersion: apps/v1and includes a properselectorfield matching the pod template labels. Please double-check that the indentation of thetemplate:and its nestedspec:sections is consistent with Kubernetes configurations. Additionally, while plaintext credentials are acceptable for CI testing, consider externalizingMINIO_ACCESS_KEYandMINIO_SECRET_KEYinto secrets when moving to production environments.🧰 Tools
🪛 actionlint (1.7.4)
1-1: unexpected key "apiVersion" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
1-1: "on" section is missing in workflow
(syntax-check)
1-1: "jobs" section is missing in workflow
(syntax-check)
2-2: unexpected key "kind" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
3-3: unexpected key "metadata" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
5-5: unexpected key "spec" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
27-38: Service Resource: Confirm Static Configuration and Consider Flexibility
The Service resource is correctly defined with the necessary selector and port mappings. Note that the static assignment ofclusterIP: 10.96.3.3works for your testing environment but may lead to conflicts or reduce flexibility in other environments. If possible, consider letting Kubernetes assign the cluster IP automatically (or configure it via a dedicated parameter).🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 38-38: no new line character at the end of file
(new-line-at-end-of-file)
38-38: Trailing Newline: Adhere to Best Practices
YAMLlint has flagged that there is no newline at the end of the file. Adding a newline after line 38 will improve compatibility with tools and is a best practice.Below is a small diff to correct this:
- clusterIP: 10.96.3.3 \ No newline at end of file + clusterIP: 10.96.3.3 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 38-38: no new line character at the end of file
(new-line-at-end-of-file)
1-38: Linter Configuration: Suppress Actionlint Warnings for Non-Workflow Files
Actionlint is reporting unexpected keys such asapiVersion,kind, andmetadatabecause it expects GitHub Actions workflow syntax. Since this file is a Kubernetes manifest used in your CI/CD pipeline, consider configuring Actionlint (or your linter settings) to ignore files in the.github/workflows/assets/directory.🧰 Tools
🪛 actionlint (1.7.4)
1-1: unexpected key "apiVersion" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
1-1: "on" section is missing in workflow
(syntax-check)
1-1: "jobs" section is missing in workflow
(syntax-check)
2-2: unexpected key "kind" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
3-3: unexpected key "metadata" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
5-5: unexpected key "spec" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🪛 YAMLlint (1.35.1)
[error] 38-38: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/assets/minio.yaml(1 hunks).github/workflows/ci.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/assets/minio.yaml
1-1: unexpected key "apiVersion" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
1-1: "on" section is missing in workflow
(syntax-check)
1-1: "jobs" section is missing in workflow
(syntax-check)
2-2: unexpected key "kind" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
3-3: unexpected key "metadata" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
5-5: unexpected key "spec" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🪛 YAMLlint (1.35.1)
.github/workflows/assets/minio.yaml
[error] 38-38: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tests.py (1)
34-37: Looks good! Consider adding a docstring.The addition of the
endpoint_urlproperty to specify the MinIO server endpoint is appropriate for the test class. This aligns with the PR objectives to improve API exception handling, as it helps set up proper testing infrastructure.Consider adding a docstring to explain the purpose of this property and its relationship to the MinIO testing setup.
@property def endpoint_url(self): + """Return the MinIO endpoint URL used for testing.""" return "http://host.docker.internal:9000"🧰 Tools
🪛 GitHub Actions: CI
[warning] 37-37: Flake8: W292 no newline at end of file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)tests/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
tests/tests.py
🪛 GitHub Actions: CI
tests/tests.py
[warning] 37-37: Flake8: W292 no newline at end of file
🤖 I have created a release *beep* *boop* --- ## [0.4.0](v0.3.2...v0.4.0) (2025-04-04) ### Features * use k8s job API and improve status check robustness in case of injected containers ([#43](#43)) ([1ff6927](1ff6927)) ### Bug Fixes * Added documentation for scale variable ([#40](#40)) ([1b668c1](1b668c1)) * properly catch and report ApiExceptions ([#42](#42)) ([92375e6](92375e6)) --- 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>
Summary by CodeRabbit
Bug Fixes
New Features