Added BaseService abstract class and utilities.#7
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a foundational BaseService abstract class and utility functions to establish a consistent pattern for interacting with the Terraform Enterprise/Cloud API. The implementation provides standardized HTTP request handling, JSONAPI serialization/deserialization, and common service operations.
- Adds BaseService abstract class with generic typing for consistent API service patterns
- Implements utility functions for JSONAPI data handling and query parameter building
- Provides comprehensive unit test coverage for both new modules
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tfe/utils.py | Utility functions for JSONAPI serialization, deserialization, and query parameter building |
| tfe/base_service.py | Abstract base service class providing HTTP request handling and common CRUD operations |
| tfe/init.py | Export BaseService in public API |
| tests/units/test_utils.py | Comprehensive unit tests for utility functions |
| tests/units/test_base_service.py | Unit tests for BaseService functionality with mock implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| params: dict[str, Any] = {} | ||
| for key, value in kwargs.items(): | ||
| if value is not None: | ||
| if isinstance(value, list | tuple): |
There was a problem hiding this comment.
The union syntax list | tuple requires Python 3.10+. For broader compatibility, use isinstance(value, (list, tuple)) instead.
| if isinstance(value, list | tuple): | |
| if isinstance(value, (list, tuple)): |
| def list(self, **kwargs: Any) -> list[T]: | ||
| """List resources instances.""" |
There was a problem hiding this comment.
The docstring has a grammatical error. Change 'List resources instances' to 'List resource instances'.
| def list(self, **kwargs: Any) -> list[T]: | |
| """List resources instances.""" | |
| """List resource instances.""" |
| For example, OrganizationsService would be BaseService[Organization]. | ||
| """ | ||
|
|
||
| def __init__(self, client: Any) -> None: |
There was a problem hiding this comment.
We shouldn't use type any for client when we have a client class defined
| T = TypeVar("T") | ||
|
|
||
|
|
||
| class BaseService(ABC, Generic[T]): |
There was a problem hiding this comment.
Why do we need a generic here ?
| http_client = self.client.config.http_client | ||
|
|
||
| # Log the request | ||
| self._log_request(method, path, **kwargs) |
There was a problem hiding this comment.
Probably not good to log **kwargs in case they contain sensitive data
| Args: | ||
| method: HTTP method (GET, POST, PUT, PATCH, DELETE) | ||
| path: API path (will be joined with base_url) | ||
| **kwargs: Additional arguments to pass to the HTTP client |
There was a problem hiding this comment.
What additional params are we currently expecting?
| else: | ||
| raise ValueError(f"Unsupported HTTP method: {method}") | ||
|
|
||
| def _handle_response(self, response: Any, model_class: type | None = None) -> Any: |
There was a problem hiding this comment.
Response should have a defined type
| from typing import Any | ||
|
|
||
|
|
||
| def deserialize_jsonapi(data: dict[str, Any], model_class: type) -> Any: |
There was a problem hiding this comment.
we shouldn't return type Any
| @@ -0,0 +1,426 @@ | |||
| """Unit tests for the BaseService abstract class.""" | |||
|
|
|||
| from unittest.mock import Mock | |||
There was a problem hiding this comment.
we should use pytest-mock
Description
Added BaseService abstract class and basic utilities.
Testing plan
Unit tests added
External links
Output from tests
Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.
Rollback Plan
Changes to Security Controls
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.
If you have any questions, please contact your direct supervisor, GRC (#team-grc), or the PCI working group (#proj-pci-reboot). You can also find more information at PCI Compliance.