Conversation
… run which The sub-project's uv environment does not contain protoc-gen-connect-python, so `uv run which` failed and silently aborted buf generate. Now checks the parent repo's .venv first, falling back to the system PATH.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the protobuf definitions to align with version 0.12.0 of the OpenTDF platform, introducing features such as obligation fulfillment context, namespace certificates, and an allow_traversal flag for attribute definitions. It also adds a manager field to key provider configurations and implements validation rules across various policy and authorization messages. Feedback identifies misleading documentation in the ValueTriggerRequest message regarding entity identification and suggests improving the cross-platform portability of the plugin resolution logic in the generation script by using shutil.which().
| // Required. The ID of the action that will trigger this obligation value policy decisioning. | ||
| common.IdNameIdentifier action = 1 [(buf.validate.field).required = true]; | ||
| // Required. The attribute value ID that will trigger this obligation value policy decisioning. | ||
| common.IdFqnIdentifier attribute_value = 2 [(buf.validate.field).required = true]; |
There was a problem hiding this comment.
The comments for the action and attribute_value fields in ValueTriggerRequest are misleading. They mention 'ID', but the types IdNameIdentifier and IdFqnIdentifier allow identifying the entities by either ID or Name/FQN. The comments should be updated to reflect this flexibility.
| # Resolve the connect-python plugin path. The buf.gen.yaml uses a relative | ||
| # path (e.g. ../.venv/bin/protoc-gen-connect-python) that is correct for | ||
| # running buf directly, but we need an absolute path when rewriting the file. | ||
| # Try the known relative location first; fall back to searching the PATH. | ||
| candidate = ( | ||
| proto_gen_dir / ".." / ".venv" / "bin" / "protoc-gen-connect-python" | ||
| ).resolve() | ||
| if candidate.exists(): | ||
| connect_plugin_path = str(candidate) | ||
| else: | ||
| result = subprocess.run( | ||
| ["which", "protoc-gen-connect-python"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| connect_plugin_path = result.stdout.strip() |
There was a problem hiding this comment.
The logic for resolving the protoc-gen-connect-python plugin path is not cross-platform. It hardcodes the bin directory (which is Scripts on Windows) and uses the Unix-specific which command as a fallback. Using shutil.which() would be more portable and idiomatic for searching the PATH.
1 participant
No description provided.