Refactor CLI to use generated OpenAPI client for mgmt_api
#311
Conversation
- Refactor CLI commands to support dependency injection via functional options pattern - Add DrasiClient interface to enable mocking of Management API - Create testutil package with mock implementations for DrasiClient, PlatformClient, and TaskOutput - Add comprehensive unit tests covering success/failure paths for all resource management commands - Fix error handling bugs: commands now properly return errors instead of printing - Fix output routing: use cmd.OutOrStdout() for testability - Individual command coverage: list 78.3%, describe 78.6%, apply 84.4%, delete 84.4% Signed-off-by: Aman Singh <aman.singh.original@gmail.com>
This commit refactors the CLI's API client to use a strongly-typed Go client generated from the management API's OpenAPI specification. This improves maintainability, provides compile-time type safety, and reduces manual boilerplate code. The new implementation delegates all standard RESTful operations to the generated client while preserving the custom logic for non-RESTful endpoints. Key changes include: - Adds `oapi-codegen` as the tool for generating the Go client from the OpenAPI spec. - Introduces a `make generate-api-client` target to the `cli/Makefile` to automate the generation process. - Refactors `sdk.ApiClient` to wrap the generated client. - All RESTful methods (`Apply`, `Delete`, `GetResource`, `ListResources`) now use the generated client. - The existing manual implementations for `Watch` (streaming) and `ReadyWait` (long-polling) are preserved as they handle logic unsupported by the generator. - The generated client code is committed to `cli/sdk/generated/` to ensure build reproducibility. Signed-off-by: Aman Singh <aman.singh.original@gmail.com>
amansinghoriginal
force-pushed
the
cli_spec
branch
from
8bdd6c4 to
e849055
Compare
amansinghoriginal
requested review from
agentofreality,
Copilot,
danielgerlag and
ruokun-niu
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the Drasi CLI to use a generated OpenAPI client for the management API, replacing manual HTTP request logic with a strongly-typed, compile-time safe client implementation.
- Introduces generated API client integration using
oapi-codegentool - Refactors existing
ApiClientto use the generated client for standard REST operations - Adds comprehensive unit tests for CLI commands with dependency injection
- Updates build processes and CI configuration to support the new architecture
Reviewed Changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/sdk/api_client.go | Core refactoring to integrate generated OpenAPI client with helper methods for routing operations |
| cli/sdk/platform_client.go | Updated to initialize generated client and changed return type to interface |
| cli/cmd/*.go | Added dependency injection support for testability in apply, delete, describe, and list commands |
| cli/testutil/*.go | New mock implementations for testing CLI components |
| cli/cmd/*_test.go | Comprehensive unit tests for CLI commands |
| cli/Makefile | Added target for generating API client from OpenAPI spec |
| .github/workflows/build-test.yml | Added CLI testing workflow and updated Go version |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| output.FailTask(subject, fmt.Sprintf("Error: %v: %v", subject, err.Error())) | ||
| return err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
The defer resp.Body.Close() statement is called immediately after the HTTP request, but the response body is read later in the function at line 158. This will close the body before it can be read, causing the read operation to fail.
| output.FailTask(subject, fmt.Sprintf("Error: %v: %v", subject, err.Error())) | ||
| return err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Same issue as in the Apply method - the response body is being closed with defer immediately after the request, but the body needs to be read later in the function. This will cause the body read operation to fail.
| if err != nil { | ||
| fmt.Println("Error: " + err.Error()) | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
[nitpick] The error handling has been improved by returning the error directly instead of printing and returning nil, but this changes the behavior from the previous implementation. Consider whether this change in error handling behavior is intentional.
| if err != nil { | ||
| fmt.Println("Error: " + err.Error()) | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
[nitpick] Similar to the list command, the error handling behavior has changed from printing the error and returning nil to returning the error directly. Ensure this behavioral change is intentional and consistent with the overall CLI error handling strategy.
2 participants
Description
This pull request refactors the Drasi CLI's
ApiClientto use a strongly-typed Go client generated directly from the management API's OpenAPI specification.The primary goal is to improve the long-term maintainability and robustness of the CLI by replacing the manual, string-based HTTP request logic with a compile-time safe, auto-generated client. This work leverages the automated OpenAPI spec generation introduced in a previous commit and completes the API contract workflow.
Key Changes
1. Generated API Client Integration
The core of this work was to replace the manual REST API calls in
cli/sdk/api_client.gowith a generated client.make generate-api-clienttarget has been added to thecli/Makefile. It uses theoapi-codegentool to generate a Go client from theopenapi.jsonfile produced by themgmt_api.cli/sdk/generated/api.gen.go. This is a Go best practice that ensures build reproducibility and decouples the build process from the generator tool.sdk.ApiClienthas been refactored to wrap and delegate all standard RESTful calls (Apply,Delete,GetResource,ListResources) to the new, strongly-typed generated client.2. Preservation of Non-RESTful Logic
A key part of the implementation was to ensure that endpoints with custom behavior were not broken by the move to a standard REST client generator.
Watch(Streaming): The existing manual implementation for theWatchcommand has been preserved, as it requires special logic to handle a long-lived, streaming HTTP response.ReadyWait(Long-Polling): Similarly, theReadyWaitcommand's logic, which uses a custom client timeout for long-polling, remains untouched.3. Code Quality and Safety Improvements
kind(e.g., "Source", "Reaction") to the correct generated client function has been centralized into a set of private helper methods (putResource,getResource, etc.).Type of change
This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Drasi.