- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
feat!: rewrite in pure Python #62
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
- 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
Cleanup tests
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* 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: 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
* 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
* 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
* 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 <[email protected]>
| 
           /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 is a massive and impressive rewrite of the library from a gopy-based solution to a pure Python implementation. The changes modernize the toolchain by adopting uv, buf, and Connect RPC, which significantly improves maintainability, compatibility, and the overall development experience. The addition of comprehensive documentation for the new architecture, development setup, and release process is commendable. The CI/CD pipeline improvements, including the separation of unit and integration tests and automated log collection on failure, are excellent additions. I have a few suggestions to further improve the robustness and security of the scripts.
| @@ -0,0 +1,56 @@ | |||
| #!/bin/bash | |||
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.
| echo "" | ||
| 
               | 
          ||
| get_token() { | ||
| curl -k --location "$TOKEN_URL" \ | 
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 use of curl -k (and grpcurl -plaintext on line 39) disables certificate validation, which is a security risk. While this may be acceptable for a local test environment, it would be beneficial to add a comment explaining why this is necessary and to warn against using this script in production-like environments.
| } | ||
| 
               | 
          ||
| echo "🔐 Getting access token..." | ||
| BEARER=$( get_token | jq -r '.access_token' ) | 
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.
It's good practice to check if the BEARER token was successfully retrieved. If the curl or jq command fails, the script will continue with an empty token, which could lead to confusing errors later.
| BEARER=$( get_token | jq -r '.access_token' ) | |
| BEARER=$( get_token | jq -r '.access_token' ) | |
| if [ -z "$BEARER" ]; then | |
| echo "❌ Failed to get access token." | |
| exit 1 | |
| fi | 
| git clone https://github.com/opentdf/platform.git | ||
| fi | ||
| cd platform | ||
| git checkout 3360befcb3e6e9791d7bfd2e89128aee0e7d2818 # Branch 'DSPX-1539-keytoolnomore' | 
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.
| } | ||
| } | ||
| ' opentdf.yaml | ||
| chmod -R 700 ./keys | 
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 permission 700 grants execute permissions to files, which is unnecessary for keys and could be a minor security risk. A more secure approach is to set 700 for directories and 600 for files. You can achieve this with:
find ./keys -type d -exec chmod 700 {} +;
find ./keys -type f -exec chmod 600 {} +;| Creating a helper function may simplify the usage of `otdf-python`. | ||
| 1. Clone the repository: | ||
| ```bash | ||
| git clone <repository-url> | 
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.
| def _fix_ignore_if_default_value(proto_files_dir): | ||
| """ | ||
| TODO: Fix buf validation: Updated the proto files to use the correct enum value: | ||
| Changed IGNORE_IF_DEFAULT_VALUE → IGNORE_IF_ZERO_VALUE in: | ||
| attributes.proto | ||
| key_access_server_registry.proto | ||
| namespaces.proto | ||
| See release notes: | ||
| * https://github.com/bufbuild/protovalidate/releases/tag/v0.14.2 | ||
| > IGNORE_IF_DEFAULT_VALUE is removed. | ||
| > In most cases, you can replace it with IGNORE_IF_ZERO_VALUE. See #396 for details. | ||
| > https://github.com/bufbuild/protovalidate/pull/396 | ||
| """ | ||
| # raise NotImplementedError | ||
| 
               | 
          ||
| # Iterate all .proto files in the directory | ||
| for proto_file in proto_files_dir.glob("**/*.proto"): | ||
| try: | ||
| with open(proto_file, "r") as file: # noqa: UP015 | ||
| content = file.read() | ||
| 
               | 
          ||
| # Replace the old enum value with the new one | ||
| updated_content = content.replace( | ||
| "IGNORE_IF_DEFAULT_VALUE", "IGNORE_IF_ZERO_VALUE" | ||
| ) | ||
| 
               | 
          ||
| # Write the updated content back to the file | ||
| with open(proto_file, "w") as file: | ||
| file.write(updated_content) | ||
| 
               | 
          ||
| print(f"Updated {proto_file.name} to use IGNORE_IF_ZERO_VALUE") | ||
| 
               | 
          ||
| except Exception as e: # noqa: PERF203 | ||
| print(f"Error updating {proto_file.name}: {e}") | ||
| 
               | 
          
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 function to patch the .proto files is a clever workaround for the deprecated IGNORE_IF_DEFAULT_VALUE enum. While pragmatic, this suggests the upstream proto definitions from protovalidate are using a deprecated feature. Consider opening an issue in the upstream repository to encourage a fix, which would allow you to remove this workaround in the future and rely on pristine source files.
* 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>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* chore: fix release-please config * chore: remove invalid changelog entries * chore: roll back to 0.3.0
- 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
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Alpha release
An alpha release is available now: https://test.pypi.org/project/otdf-python/0.3.0a6/
✨🪩✨ New and Improved! ✨🪩✨
This pull request rewrites this library in "pure python" (of course, using the
cryptographylibrary means that Rust is used under the hood for crypto).In this PR, we introduce a simplified, modernized toolchain. In particular, the CI pipeline and local development is improved using using the
uvtool. The most important changes are grouped below.Build and Test Improvements:
Switches to
uvfor Python environment management. This simplifies the CI/CD process, improve maintainability, and align the workflows with modern Python tooling and best practices.Switches to building a sub-module using
bufand generating Connect RPC client code.buf#45bufbuild #50Deletes obsolete workflows related to
gopytoolingMaintains and expands integration testing: separates unit and integration tests (can now be run with
uv run pytest, and can execute tests with clear markers).Increased compatibility:
With the updated build, we will be able to broadly support macOS, Linux, and Windows
Currently, we are supporting Python version 3.10+
TODO
_get_token_from_client_credentials)TODO (nice to have)
realm_name = "opentdf") to configuration loader, log a warning when we fallbackself.issuer_endpoint, to align more closely with Java SDK's SDKBuilder.java (getplatform_issuerfrom well known configuration)ruffrules ( e.g."D"for pydocstyle [ docstring conventions] )getattrandkwargs