Conversation
sid-rl
left a comment
There was a problem hiding this comment.
overall looks good! a couple things:
- there's a couple places where double quotes should be used instead of single quotes in the docstrings. running
./scripts/formatshould automatically fix this - missing smoketests
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_gateway_config_update(self, sdk_client: RunloopSDK) -> None: | ||
| """Test updating a gateway config.""" | ||
| gateway_config = sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-gateway-config-update"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| description="Original description", | ||
| ) | ||
|
|
||
| try: | ||
| # Update the gateway config | ||
| updated_name = unique_name("sdk-gateway-config-updated") | ||
| result = gateway_config.update( | ||
| name=updated_name, | ||
| description="Updated description", | ||
| ) | ||
|
|
||
| assert result is not None | ||
| assert result.name == updated_name | ||
| assert result.description == "Updated description" | ||
|
|
||
| # Verify update persisted | ||
| info = gateway_config.get_info() | ||
| assert info.name == updated_name | ||
| assert info.description == "Updated description" | ||
| finally: | ||
| gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_gateway_config_update_endpoint(self, sdk_client: RunloopSDK) -> None: | ||
| """Test updating gateway config endpoint.""" | ||
| gateway_config = sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-gateway-config-endpoint"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| ) | ||
|
|
||
| try: | ||
| result = gateway_config.update( | ||
| endpoint="https://api.updated-example.com", | ||
| ) | ||
|
|
||
| assert result.endpoint == "https://api.updated-example.com" | ||
|
|
||
| # Verify update persisted | ||
| info = gateway_config.get_info() | ||
| assert info.endpoint == "https://api.updated-example.com" | ||
| finally: | ||
| gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_gateway_config_update_auth_mechanism(self, sdk_client: RunloopSDK) -> None: | ||
| """Test updating gateway config auth mechanism.""" | ||
| gateway_config = sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-gateway-config-auth"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| ) | ||
|
|
||
| try: | ||
| result = gateway_config.update( | ||
| auth_mechanism={"type": "header", "key": "x-api-key"}, | ||
| ) | ||
|
|
||
| assert result.auth_mechanism.type == "header" | ||
| assert result.auth_mechanism.key == "x-api-key" | ||
|
|
||
| # Verify update persisted | ||
| info = gateway_config.get_info() | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "x-api-key" | ||
| finally: | ||
| gateway_config.delete() |
There was a problem hiding this comment.
could probably consolidate all of these into one smoketest
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_gateway_config_with_header_auth(self, sdk_client: RunloopSDK) -> None: | ||
| """Test creating a gateway config with header auth.""" | ||
| gateway_config = sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-gateway-header"), | ||
| endpoint="https://api.header-test.com", | ||
| auth_mechanism={"type": "header", "key": "x-api-key"}, | ||
| ) | ||
|
|
||
| try: | ||
| info = gateway_config.get_info() | ||
| assert info.auth_mechanism is not None | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "x-api-key" | ||
| finally: | ||
| gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_gateway_config_with_authorization_header(self, sdk_client: RunloopSDK) -> None: | ||
| """Test creating a gateway config with Authorization header.""" | ||
| gateway_config = sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-gateway-auth-header"), | ||
| endpoint="https://api.auth-header-test.com", | ||
| auth_mechanism={"type": "header", "key": "Authorization"}, | ||
| ) | ||
|
|
||
| try: | ||
| info = gateway_config.get_info() | ||
| assert info.auth_mechanism is not None | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "Authorization" | ||
| finally: | ||
| gateway_config.delete() |
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_gateway_config_update(self, async_sdk_client: AsyncRunloopSDK) -> None: | ||
| """Test updating a gateway config.""" | ||
| gateway_config = await async_sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-async-gateway-config-update"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| description="Original async description", | ||
| ) | ||
|
|
||
| try: | ||
| # Update the gateway config | ||
| updated_name = unique_name("sdk-async-gateway-config-updated") | ||
| result = await gateway_config.update( | ||
| name=updated_name, | ||
| description="Updated async description", | ||
| ) | ||
|
|
||
| assert result is not None | ||
| assert result.name == updated_name | ||
| assert result.description == "Updated async description" | ||
|
|
||
| # Verify update persisted | ||
| info = await gateway_config.get_info() | ||
| assert info.name == updated_name | ||
| assert info.description == "Updated async description" | ||
| finally: | ||
| await gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_gateway_config_update_endpoint(self, async_sdk_client: AsyncRunloopSDK) -> None: | ||
| """Test updating gateway config endpoint.""" | ||
| gateway_config = await async_sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-async-gateway-config-endpoint"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| ) | ||
|
|
||
| try: | ||
| result = await gateway_config.update( | ||
| endpoint="https://api.updated-example.com", | ||
| ) | ||
|
|
||
| assert result.endpoint == "https://api.updated-example.com" | ||
|
|
||
| # Verify update persisted | ||
| info = await gateway_config.get_info() | ||
| assert info.endpoint == "https://api.updated-example.com" | ||
| finally: | ||
| await gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_gateway_config_update_auth_mechanism(self, async_sdk_client: AsyncRunloopSDK) -> None: | ||
| """Test updating gateway config auth mechanism.""" | ||
| gateway_config = await async_sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-async-gateway-config-auth"), | ||
| endpoint="https://api.example.com", | ||
| auth_mechanism={"type": "bearer"}, | ||
| ) | ||
|
|
||
| try: | ||
| result = await gateway_config.update( | ||
| auth_mechanism={"type": "header", "key": "x-api-key"}, | ||
| ) | ||
|
|
||
| assert result.auth_mechanism.type == "header" | ||
| assert result.auth_mechanism.key == "x-api-key" | ||
|
|
||
| # Verify update persisted | ||
| info = await gateway_config.get_info() | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "x-api-key" | ||
| finally: | ||
| await gateway_config.delete() |
There was a problem hiding this comment.
consolidate into one smoketest
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_gateway_config_with_header_auth(self, async_sdk_client: AsyncRunloopSDK) -> None: | ||
| """Test creating a gateway config with header auth.""" | ||
| gateway_config = await async_sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-async-gateway-header"), | ||
| endpoint="https://api.header-test.com", | ||
| auth_mechanism={"type": "header", "key": "x-api-key"}, | ||
| ) | ||
|
|
||
| try: | ||
| info = await gateway_config.get_info() | ||
| assert info.auth_mechanism is not None | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "x-api-key" | ||
| finally: | ||
| await gateway_config.delete() | ||
|
|
||
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_gateway_config_with_authorization_header(self, async_sdk_client: AsyncRunloopSDK) -> None: | ||
| """Test creating a gateway config with Authorization header.""" | ||
| gateway_config = await async_sdk_client.gateway_config.create( | ||
| name=unique_name("sdk-async-gateway-auth-header"), | ||
| endpoint="https://api.auth-header-test.com", | ||
| auth_mechanism={"type": "header", "key": "Authorization"}, | ||
| ) | ||
|
|
||
| try: | ||
| info = await gateway_config.get_info() | ||
| assert info.auth_mechanism is not None | ||
| assert info.auth_mechanism.type == "header" | ||
| assert info.auth_mechanism.key == "Authorization" | ||
| finally: | ||
| await gateway_config.delete() |
🤖 PR Review Agent - StatusStatus: 🟢 Plan approved - Implementation in progress SSH in to interact: Last updated: 2026-02-04 01:55:17 UTC |
🤖 PR Review Agent - ImplementingStatus: 🟢 Plan Approved - Implementation in ProgressPR Review PlanSummaryThis PR adds gateway configuration support to the SDK, including synchronous and asynchronous operations classes (GatewayConfigOps/AsyncGatewayConfigOps), resource wrapper classes (GatewayConfig/AsyncGatewayConfig), unit tests, and smoketests. It also enhances the lint script to include format checking. Issues FoundConvention Violations
KISS Violations
Code DuplicationNone found beyond expected sync/async duplication patterns that are consistent with the existing codebase architecture. CI/Build ErrorsUnable to run linter/tests locally due to missing
Reviewer FeedbackAll reviewer comments addressed in the proposed changes below:
Proposed Changes1. Run code formatter
2. Consolidate auth mechanism tests (sync version)
3. Consolidate auth mechanism tests (async version)
No Changes NeededThe core implementation (gateway_config.py, async_gateway_config.py, sync.py, async_.py) follows existing codebase patterns correctly and doesn't require changes beyond what the formatter will handle automatically.
Implementation started. Claude is now making the changes... SSH in to interact: Last updated: 2026-02-04 01:55:16 UTC |
Consolidate redundant auth mechanism tests into parameterized tests: - Replace 3 separate test methods with 1 parameterized test per file - Remove duplicate test_gateway_config_with_authorization_header - Maintain equivalent test coverage with less code duplication Addresses reviewer feedback on PR #736. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
PR Review CompleteChanges MadeSuccessfully addressed all reviewer feedback for PR #736: 1. Consolidated Auth Mechanism Tests (Sync)
2. Consolidated Auth Mechanism Tests (Async)
3. Code Formatting
CommitsCommit:
Verification
SummaryAll reviewer comments have been addressed:
The changes reduce code duplication, improve maintainability, and address all KISS violations identified in the review. The parameterized approach is more concise and follows pytest best practices. |
No description provided.