-
Notifications
You must be signed in to change notification settings - Fork 1
TF-30335 Add Foundational SDK Core Framework and Structure #9
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
| @@ -0,0 +1,9 @@ | |||
| from tfe._http import HTTPTransport | |||
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.
I understand you have deleted the old tests because the new codebase has new class names, locations, and API surfaces, so the old tests would fail. Ensure you have a ticket for this at a later time.
src/tfe/_http.py
Outdated
|
|
||
| async def _asleep(self, attempt: int, retry_after: float | None): | ||
| if retry_after is not None: await anyio.sleep(retry_after); return | ||
| delay = min(self.backoff_cap, self.backoff_base * (2 ** attempt)) |
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.
backoff_jitter is accepted in init but not applied here. Consider adding random jitter to the delay.
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.
Ignore async methods, Async are not thoroughly tested.
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.
Pull Request Overview
This PR introduces a comprehensive rewrite of the python-tfe library to establish a modern, typed SDK foundation for Terraform Enterprise/Cloud. The changes include migrating to Pydantic-based models, implementing robust HTTP transport with retry logic, and restructuring the project for better maintainability and extensibility.
- Replaced simple dataclass configuration with Pydantic-based
TFEConfigand comprehensive error handling - Introduced
HTTPTransportwith retry policies and proper JSON:API support - Created typed domain models and resource services for Organizations, Projects, and Workspaces
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated build system to hatchling, changed package name to "tfe", added new dependencies |
| src/tfe/init.py | New module exports for TFEConfig, TFEClient, and errors |
| src/tfe/config.py | New Pydantic-based configuration with environment variable support |
| src/tfe/client.py | New TFEClient implementation using HTTPTransport |
| src/tfe/_http.py | HTTP transport layer with retry logic and error mapping |
| src/tfe/errors.py | Comprehensive error hierarchy for different API conditions |
| src/tfe/types.py | Pydantic models for core domain objects |
| src/tfe/resources/*.py | Resource service implementations for Organizations, Projects, Workspaces |
| examples/ws_list.py | Example usage of the new SDK |
| tests/units/*.py | Updated test files for new structure |
Comments suppressed due to low confidence (1)
pyproject.toml:1
- The classifier for Python 3.9 is missing but the project requires Python >=3.9. Add
'Programming Language :: Python :: 3.9',to the classifiers list.
[build-system]
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return ExecutionMode._value2member_map_.get(v) # type: ignore[return-value] | ||
|
|
||
|
|
Copilot
AI
Sep 8, 2025
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.
Accessing private attribute _value2member_map_ is not recommended. Use the public try/except ValueError pattern with ExecutionMode(v) instead.
| return ExecutionMode._value2member_map_.get(v) # type: ignore[return-value] | |
| try: | |
| return ExecutionMode(v) | |
| except ValueError: | |
| return None |
| self._sync = httpx.Client( | ||
| http2=http2, timeout=timeout, verify=ca_bundle or verify_tls | ||
| ) # proxies=proxies | ||
| self._async = httpx.AsyncClient( | ||
| http2=http2, timeout=timeout, verify=ca_bundle or verify_tls | ||
| ) # proxies=proxies |
Copilot
AI
Sep 8, 2025
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 commented out proxies=proxies suggests incomplete implementation. Either implement proxy support or remove the commented code and the proxies parameter if not needed.
| self._sync = httpx.Client( | |
| http2=http2, timeout=timeout, verify=ca_bundle or verify_tls | |
| ) # proxies=proxies | |
| self._async = httpx.AsyncClient( | |
| http2=http2, timeout=timeout, verify=ca_bundle or verify_tls | |
| ) # proxies=proxies | |
| client_kwargs = { | |
| "http2": http2, | |
| "timeout": timeout, | |
| "verify": ca_bundle or verify_tls, | |
| } | |
| if proxies is not None: | |
| client_kwargs["proxies"] = proxies | |
| self._sync = httpx.Client(**client_kwargs) | |
| self._async = httpx.AsyncClient(**client_kwargs) |
|
|
||
|
|
||
| """ | ||
| Warning: Do Not Use this Async Service as its not stable with HashiCorp API. |
Copilot
AI
Sep 8, 2025
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.
Grammar error: 'its' should be 'it's' (it is). Should read: 'Warning: Do Not Use this Async Service as it's not stable with HashiCorp API.'
| Warning: Do Not Use this Async Service as its not stable with HashiCorp API. | |
| Warning: Do Not Use this Async Service as it's not stable with HashiCorp API. |
mukulmohan177
left a comment
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 looks good. The core abstractions (TFEClient, config, transport, and resource layers) are thoughtfully designed and this can be extended in the future meeting our scalability requirements.
Summary Table
|
Description
This PR lays the core foundation for a typed, ergonomic Python SDK for Terraform Enterprise.
It establishes a consistent HTTP transport with retries, JSON:API helpers, config loading, typed domain models, and resource services plus packaging and an example. This is the baseline we’ll grow into full TFE coverage.
Note: Organizations, Projects, Workspaces resources added here are not final draft.
Key Changes
src/tfe/client.py--> TFEClient:Wires HTTPTransport from TFEConfig.
src/tfe/resources/_base.py--> base classes with pagination helpers. This need to be tested for all APIs.src/tfe/types.py— Pydantic models and enums.