Initial Client and Config class for tfe#5
Conversation
| self.address = f"https://{host}" | ||
| else: | ||
| self.address = DEFAULT_ADDRESS | ||
|
|
There was a problem hiding this comment.
Hey why don't we check here if the string is actually a valid URL? Add a validator here.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the foundational Client and Config classes for the python-tfe library, replacing a placeholder implementation with a production-ready API client structure.
- Implements a comprehensive Config class with environment variable support and validation
- Creates a Client class that fetches API metadata and provides cloud/enterprise detection
- Restructures the project from src/python_tfe to tfe package layout
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tfe/config.py | New Config class with environment variable handling, token management, and validation |
| tfe/client.py | New Client class with API metadata fetching and instance type detection |
| tfe/init.py | Updates exports to use new Client and Config classes |
| tests/units/test_config.py | Comprehensive unit tests for Config class functionality |
| tests/units/test_client.py | Unit tests for Client class with mocked HTTP responses |
| tests/test_client.py | Removes old placeholder test file |
| src/python_tfe/client.py | Removes old placeholder client implementation |
| pyproject.toml | Updates package structure and adds new dev dependencies |
| Makefile | Updates source directory reference and test verbosity |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,17 +1 @@ | |||
| """ | |||
There was a problem hiding this comment.
I noticed that the tests for the client were removed in this PR, but no new tests were added for the new Client class. Can you explain the reasoning behind removing these tests?
There was a problem hiding this comment.
Yeah I had just moved the tests around, so earlier the tests were in /tests however later i moved them to /tests/units
| assert cfg.base_path == config.DEFAULT_BASE_PATH | ||
| assert cfg.registry_base_path == config.DEFAULT_REGISTRY_PATH | ||
| assert cfg.token is None | ||
| assert cfg.token == "" |
There was a problem hiding this comment.
One thing to keep in mind is our code must consistently treat "" as “missing.”
There was a problem hiding this comment.
Yes, I think it will be better to say not cfg.token for anything we add like this so it is consistent
Description
Initial Client and Config class for tfe
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