-
Notifications
You must be signed in to change notification settings - Fork 0
fix: guarantee target-version decrypt support #84
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
Conversation
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.
Summary of Changes
Hello @b-long, 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 significantly improves the testing framework by adding robust support for generating and validating TDF files against specific target versions. It introduces new fixtures to create versioned TDF data and new integration tests to ensure the CLI's inspection capabilities are compatible with these different formats. Additionally, it centralizes configuration handling for cleaner and more efficient test setups.
Highlights
- Enhanced Testing for TDF Target Versions: Introduced new pytest fixtures (
tdf_v4_2_2_files,tdf_v4_3_1_files,all_target_mode_tdf_files) inconftest.pyto generate TDF files encrypted with specific target versions (v4.2.2 and v4.3.1) using theotdfctltool. This significantly improves the ability to test compatibility across different TDF specifications. - New CLI Inspect Integration Tests: Added
test_cli_inspect.pywith dedicated tests to verify theotdf_pythonCLI's ability to correctly inspect TDF files created with various target versions and content types. This ensures the CLI can handle different TDF formats as expected. - Centralized Configuration Retrieval: Refactored common logic for determining platform URLs and
otdfctlcommand-line flags into new helper functions (get_platform_url,get_otdfctl_flags) withintests/support_otdfctl_args.py. This improves code reusability and maintainability across integration tests.
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 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
-
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant improvements to the testing infrastructure, particularly by adding fixtures to generate TDF files for different target versions and refactoring common test setup logic. While the direction is excellent, there are a few critical issues that need addressing. Several new tests will fail due to a dependency on test data for empty files, for which generation is currently commented out. A critical bug was also introduced in test_cli_tdf_validation.py from an incomplete refactoring, which will cause tests to run with incorrect flags. Additionally, there's an opportunity to improve maintainability by reducing code duplication in the new fixtures and to clean up test output by removing debugging print statements.
77f0269 to
63bcf8c
Compare
|
/gemini review |
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.
Code Review
This pull request significantly improves the project's testing capabilities by adding support for different TDF target versions and introducing a suite of new integration tests and fixtures. The refactoring of test configuration into a shared module and the enhancement of the inspect command's robustness are excellent changes. I've identified a few areas for improvement, primarily concerning code duplication in the new test fixtures and a minor style issue with a local import. Overall, this is a valuable contribution to the project's quality and maintainability.
|
/gemini review |
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.
Code Review
This pull request significantly improves the testing infrastructure by adding fixtures to generate TDFs for different target versions using otdfctl. This allows for better testing of target-version support. The changes also include valuable refactoring in the cli and sdk modules to improve robustness, such as better handling of missing platform URLs and more graceful failures in the inspect command. The test suite is also improved by centralizing configuration access and adding new validation tests. The overall changes are of high quality. I've found a minor issue in the logging of a new test file that I've commented on.
c3104df to
cb42f55
Compare
|
/gemini review |
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.
Code Review
This pull request significantly improves the testing infrastructure for target version support by adding a comprehensive set of integration tests. It introduces fixtures to generate TDF files for different versions and file types, along with tests to validate the inspect command's behavior with these files. The application code sees several improvements, including more robust error handling in the CLI's inspect command and stricter configuration validation in the SDK. The test code has been nicely refactored to centralize configuration-dependent logic, improving maintainability. My review found one area for simplification in the KAS URL construction logic within the SDK, which could be made more concise and readable. Overall, this is a high-quality contribution that increases confidence in the library's compatibility features.
|
/gemini review |
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.
Code Review
This pull request significantly improves the project's testing capabilities by introducing fixtures to generate and test against different TDF target versions (v4.2.2 and v4.3.1). It also enables and applies isort for consistent import ordering across the codebase, leading to numerous style changes. Key improvements include more robust SDK building in the CLI, stricter error handling in integration tests by replacing pytest.skip with exceptions, and API enhancements for flexibility. My review focuses on a few areas for further improvement, such as refining exception handling in the CLI and adjusting type hints for better API flexibility.
| builder = SDKBuilder() | ||
| if args.platform_url: | ||
| builder.set_platform_endpoint(args.platform_url) | ||
| if hasattr(args, "plaintext") and args.plaintext: | ||
| builder.use_insecure_plaintext_connection(True) | ||
| if args.insecure: | ||
| builder.use_insecure_skip_verify(True) | ||
| sdk = builder.build() |
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.
This block of code for creating a minimal SDK duplicates logic from the build_sdk function, specifically setting the platform endpoint and security flags. This duplication can lead to maintenance issues if build_sdk is updated but this block is not. Consider refactoring to reduce duplication. For example, you could create a helper function that configures a SDKBuilder with common options from args, which both build_sdk and this fallback logic can use.
| payload: bytes | BinaryIO, | ||
| config: TDFConfig, | ||
| output_stream: BinaryIO | None = None, | ||
| output_stream: io.BytesIO | None = None, |
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.
The type hint for output_stream has been changed to io.BytesIO | None, which is more restrictive than the previous BinaryIO | None. The implementation uses output_stream.write(), which is supported by any BinaryIO object. This change prevents users from streaming output directly to a file opened in binary mode (e.g., open('file.tdf', 'wb')) without a type checker warning. Reverting to BinaryIO | None would make the API more flexible and accurately reflect its capabilities.
| output_stream: io.BytesIO | None = None, | |
| output_stream: BinaryIO | None = None, |
| ) -> TDFReader: | ||
| # Extract manifest, unwrap payload key using KAS client |
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.
The type hint for tdf_data is bytes | io.BytesIO. For consistency and flexibility, it would be better to use bytes | BinaryIO. This would correctly signal that the method can accept any binary file-like object, such as one returned by open(..., 'rb'), not just io.BytesIO. The current implementation already supports this.
| self, tdf_data: bytes | io.BytesIO, config: TDFReaderConfig | |
| ) -> TDFReader: | |
| self, tdf_data: bytes | BinaryIO, config: TDFReaderConfig | |
| ) -> TDFReader: |
|
/gemini review |
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.
Code Review
This pull request introduces significant improvements to the codebase. The enabling of isort via ruff enforces consistent import ordering, which is reflected in many files. The refactoring of the integration test suite is particularly noteworthy, with the addition of fixtures for generating version-specific TDF files and helper functions for building CLI commands. This makes the tests cleaner, more maintainable, and more robust. The bug fixes in the cli.py and sdk.py modules are also valuable additions.
I've found a minor issue in one of the new test files regarding the calculation of the current working directory (cwd), which could lead to flaky tests. I've left a specific comment with a suggestion for fixing this.
* chore: update pre-commit * fix: type annotations in tdf.py * chore: expand inspect tests * chore: cleanup tests * chore: organize imports * chore: require sorted imports * chore: add test_cli_decrypt.py * chore: organize integration tests * chore: organize integration tests * Tweak attributes * chore: cleanup tests * chore: cleanup tests
* chore: dry tests * chore: relocate run_cli_inspect * chore: fix type annotation * chore: note token isn't important * chore: cleanup args & typing * chore: extract 'get_platform_url' function * chore: extract 'support_otdfctl_args' module * chore: use '*get_cli_flags()' pattern * chore: DRY code * chore: DRY code * chore: extract 'get_testing_environ' function * chore: DRY code * chore: DRY code * chore: DRY code
a993363 to
86c0fc3
Compare
* chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows
|
/gemini review |
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.
Code Review
This pull request significantly improves the project's testing infrastructure and enhances support for different TDF versions. The introduction of helper functions and data-driven fixtures for integration tests is an excellent change that will make tests more maintainable and robust. The import sorting across the codebase also improves style consistency.
My review focuses on a few areas for improvement:
- A regression in a type hint that makes a function less flexible.
- A broad exception handler in the CLI that could mask the real cause of errors.
- A suggestion to simplify some complex URL construction logic in the future.
Overall, this is a high-quality contribution with substantial improvements. Great work on the test refactoring!
| payload: bytes | BinaryIO, | ||
| config: TDFConfig, | ||
| output_stream: BinaryIO | None = None, | ||
| output_stream: io.BytesIO | None = None, |
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.
The type hint for output_stream was changed to io.BytesIO, which is more restrictive than the previous typing.BinaryIO. BinaryIO is a more general type that includes file-like objects from open(), allowing the function to write directly to files on disk, not just in-memory streams. Reverting to BinaryIO would make the function more flexible and align better with its capabilities.
| output_stream: io.BytesIO | None = None, | |
| output_stream: BinaryIO | None = None, |
* Begin rewrite in pure Python * Organize: git mv src/otdf_python/test_*.py tests/ * Format according to 'ruff' * Fix static analysis * Cleanup and organize tests/test_validate_otdf_python.py * Remove 'TDFConfig' type from 'otdf_python.tdf' * Fix description & formatting * Add 'pydantic-settings' to dev & update dependencies * Correct version number * Cleanup and fix OIDC tests * Comment old style integration test * Execute majority of tests * Allow import from 'tests' * Fix string encryption test * Remove dead code * Adjust integration test * Remove old build scripts * Update README * Update GHA triggers * Fix endpoint URL and TLS verification * ✅ Significant update 143 out of 150 tests passing - When run with the proper .env file: 7 failed, 142 passed, 2 skipped, 1 warning - Critical naming fix - Update .proto files - Add script to update .proto files - Ditch HTTP impl - Improve manifest and encrypt test - Python CLI decrypt now works correctly with TDF files created by otdfctl * Run all tests, except integration * Update GHA configuration * Mark integration tests * Fix mocked tests/test_kas_client.py * Mark integration tests * Only build for 3.13 (temporary) * Update license * Enable and fix integration tests in CI Cleanup tests * Improve support for plaintext * Make log collection optional * Fix tests for plaintext * Fix docstrings * Fix docstrings * Extract Connect RPC class * Fix additional roundtrip testing * Fix tests after kas_client updates * Expand KAS client integration tests * Fix mimeType * Expand testing, fix compression bug * Auto-use check_for_otdfctl fixture * Expand static analysis, fix FURB188 * Use 'NULL_POLICY_UUID' for now * Update kas_client.py & tdf.py, expand tests * Expand & organize integration tests * Expand static analysis, fix PT018 * Use configurable attrs in testing * Use configurable attrs in testing * Examine entitlements in CI * Extract 'temp_credentials_file' fixture * Rename file * Modernize release workflows * Modernize release workflows * Update release workflow * Manage 'otdf-python-proto' as a sub-package * Update README * Manage 'otdf-python-proto' as a sub-package * Support Python 3.10+ * Fix version number * Fix Python version requirement * Bump version 0.3.0a4 -> 0.3.0a5 * Fix version extract command * Undo file name change * More support for PE flows, cleanup & improved typing (#70) * Cleanup & improved typing * Disable odd policy enforcement * Add ".env-docker" file for local testing * Add PE test support (GHA and docker) (#71) * Add docker start script * Gemini fixes * Update GHA configuration * Gemini fixes * Enable PE e2e test * Run 'pre-commit autoupdate' & fix lint issues * Extract '_get_sdk_builder' function * Cleanup & remove redundant function * Improve typing * Use patch() context manager, reduce imports * Remove unnecessary import * Combine 'yq' expressions * Point to commit SHA * Remove hallucination * Match version number * Bump 0.3.0a5 to 0.3.0a6 * Chore/update docs and release process (#72) * Cleanup docs * Refine workflows for release management and testing - Implement `release-please` workflow for automated releases. - Create `publish-test` and `publish` workflows to handle package builds and releases. - Introduce `test-suite` workflow to run tests before publishing. - Update configuration files for release management. * Add 'ruff' as dev dependency * Configure ruff to ignore generated files * Fail fast if linting fails * Document release process * Bump version to 0.3.0a7 * Publish new alpha * Allow replacing artifacts with the same name * Remove the duplicate integration-test job * Attempt alpha release * chore: improve pre-commit configuration * chore: revert 'rm CONNECT_RPC_MIGRATION.md' * chore: disable TestPyPIBuild unless workflow_dispatch * chore: bump version 0.3.0a7 -> 0.3.0a8 * chore: bump version 0.3.0a8 -> 0.3.0a9 * chore: target this branch * chore: target develop branch * chore: fix release-please config * chore: fix version number * chore: use standard 'workflow_call' * chore: clean up publishing * fix: fix publishing * chore: release 0.3.0a10 Release-As: 0.3.0a10 * fix: fix publishing * chore: release 0.3.0a11 Release-As: 0.3.0a11 * chore: release develop (#81) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * chore: align version numbers * chore: add 'otdf-python-proto/uv.lock' file * chore: add 'otdf-python-proto/uv.lock' file * fix: omit README from Github releases * chore: document legacy version * fix: address pre-commit (lint) issues * chore: verbose output for pypi uploads * fix: use correct 'extra-files' for uv.lock See also: googleapis/release-please#2561 * chore: release 0.3.1 Release-As: 0.3.1 * chore: release develop (#82) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * chore: organize docs * fix: remove unnecessary 'ncipollo/release-action' * chore: add developer doc * chore: CI improvements (#88) * chore: prevent TestPyPI publishing <= 0.3.2 * chore: update .pre-commit-config.yaml * chore: align versions * chore: ensure future version alignment * chore: comment unused GHA step * chore: simplify version parsing * chore: add tomli for Python < 3.11 * fix: get version dynamically in 'test_cli.py' * fix: guarantee target-version decrypt support (#84) * fix: add test data * fix: improve target-version support * fix: add get_cli_flags function * fix: fix tests * fix: bug handling bytes | BinaryIO & tests * fix: update .gitignore * fix: remove invalid default KAS * fix: disable attrs for now * fix: DRY test fixtures * chore: cleanup * fix:target mode encryption (#86) * chore: update pre-commit * fix: type annotations in tdf.py * chore: expand inspect tests * chore: cleanup tests * chore: organize imports * chore: require sorted imports * chore: add test_cli_decrypt.py * chore: organize integration tests * chore: organize integration tests * Tweak attributes * chore: cleanup tests * chore: cleanup tests * chore: dry tests (#87) * chore: dry tests * chore: relocate run_cli_inspect * chore: fix type annotation * chore: note token isn't important * chore: cleanup args & typing * chore: extract 'get_platform_url' function * chore: extract 'support_otdfctl_args' module * chore: use '*get_cli_flags()' pattern * chore: DRY code * chore: DRY code * chore: extract 'get_testing_environ' function * chore: DRY code * chore: DRY code * chore: DRY code * chore: improve pre-commit config * fix: mirrored workflows for target-mode (#91) * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: cleanup for mirrored workflows * chore: remove otdf-python-proto from manifest * chore: cleanup and release (#93) * fix: don't inspect without auth * fix: process otdf-python-proto/pyproject.toml correctly * chore: remove NanoTDF from README * chore: mention legacy version in main README * chore: set version to 0.3.1 * chore: fix release-please * fix: release-please configuration (#95) * fix: "jsonpath" in release-please-config.json * chore: remove invalid changelog entries * chore: cleanup branches used in release-please * chore: remove invalid changelog file * chore: reset version to 0.3.0 * chore: cleanup whitespace * chore: improve release process * chore: document release process * chore: delete invalid information * fix: update prerelease config for develop branch * chore(develop): release otdf-python 0.3.1 (#96) * chore(develop): release otdf-python 0.3.1 * Update CHANGELOG.md --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: b-long <b-long@users.noreply.github.com> * fix: fix .release-please-config.json file (#97) * fix: fix .release-please-config.json file * chore: align for version 0.3.1 * chore: use importlib for version * chore: manage .py files without relese-please * fix: allow for development version in CLI version test * Update src/otdf_python/cli.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * chore(develop): release otdf-python 0.3.2 (#98) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix: release configuration (#99) * chore: fix release-please config * chore: remove invalid changelog entries * chore: roll back to 0.3.0 * fix: add develop-specific release-please files and update workflow - Add .release-please-config-develop.json with prerelease: true - Add .release-please-manifest-develop.json with current version - Remove dynamic file creation from workflow - Files are now committed to repo instead of generated at runtime * chore(develop): release otdf-python 0.3.1 (#100) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Test and fixture improvements, to guarantee target-version decrypt support.