add ampcode management api #453
Conversation
Add new REST API endpoints under /v0/management/ampcode for managing ampcode configuration including upstream URL, API key, localhost restriction, model mappings, and force model mappings settings. - Move force-model-mappings from config_basic to config_lists - Add GET/PUT/PATCH/DELETE endpoints for all ampcode settings - Support model mapping CRUD with upsert (PATCH) capability - Add comprehensive test coverage for all ampcode endpoints
Summary of ChangesHello @hkfires, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the 'ampcode' configuration management capabilities by introducing a dedicated and comprehensive set of API endpoints. These new routes allow for programmatic control over 'ampcode' settings such as upstream URLs, API keys, localhost access restrictions, and detailed model mappings, including the ability to get, set, update, and delete individual or collections of mappings. The changes streamline configuration management and are thoroughly tested to ensure reliability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive management API for ampcode configuration, which is a great addition. The changes are well-structured, with new handlers neatly organized and existing code refactored for clarity. The inclusion of a thorough test suite is particularly commendable, covering a wide range of scenarios for the new endpoints.
My review includes a couple of suggestions for improving the test suite's maintainability and consistency. Overall, this is a solid contribution.
| // newAmpTestHandler creates a test handler with default ampcode configuration. | ||
| func newAmpTestHandler(t *testing.T) (*management.Handler, string) { | ||
| t.Helper() | ||
| tmpDir := t.TempDir() | ||
| configPath := filepath.Join(tmpDir, "config.yaml") | ||
|
|
||
| cfg := &config.Config{ | ||
| AmpCode: config.AmpCode{ | ||
| UpstreamURL: "https://example.com", | ||
| UpstreamAPIKey: "test-api-key-12345", | ||
| RestrictManagementToLocalhost: true, | ||
| ForceModelMappings: false, | ||
| ModelMappings: []config.AmpModelMapping{ | ||
| {From: "gpt-4", To: "gemini-pro"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if err := os.WriteFile(configPath, []byte("port: 8080\n"), 0644); err != nil { | ||
| t.Fatalf("failed to write config file: %v", err) | ||
| } | ||
|
|
||
| h := management.NewHandler(cfg, configPath, nil) | ||
| return h, configPath | ||
| } |
There was a problem hiding this comment.
The test suite has a number of redundant tests. For many endpoints, there is one test that only checks for a 200 OK status (e.g., TestPutAmpUpstreamURL), and another test with a _VerifyState suffix that performs the same action and also verifies the state change (e.g., TestPutAmpUpstreamURL_VerifyState).
This pattern leads to code duplication and makes the test suite harder to maintain. It would be better to have a single, comprehensive test for each action that includes state verification.
For example, TestPutAmpUpstreamURL could be removed, and TestPutAmpUpstreamURL_VerifyState could be renamed to TestPutAmpUpstreamURL and become the sole test for that endpoint action. This principle can be applied to other similar pairs of tests in this file.
| var resp map[string]any | ||
| if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { | ||
| t.Fatalf("failed to unmarshal response: %v", err) | ||
| } | ||
|
|
||
| key := resp["upstream-api-key"].(string) | ||
| if key != "test-api-key-12345" { | ||
| t.Errorf("expected key %q, got %q", "test-api-key-12345", key) | ||
| } |
There was a problem hiding this comment.
The test TestGetAmpUpstreamAPIKey uses map[string]any to unmarshal the JSON response, which is inconsistent with other tests that use more specific types like map[string]string or map[string]bool. Using any (or interface{}) requires a type assertion, which can panic if the response format is not as expected. Using a specific type like map[string]string is safer and more consistent.
var resp map[string]string
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("failed to unmarshal response: %v", err)
}
if resp["upstream-api-key"] != "test-api-key-12345" {
t.Errorf("expected key %q, got %q", "test-api-key-12345", resp["upstream-api-key"])
}| func TestDeleteAmpModelMappings_All(t *testing.T) { | ||
| h, _ := newAmpTestHandler(t) | ||
| r := setupAmpRouter(h) | ||
|
|
||
| req := httptest.NewRequest(http.MethodDelete, "/v0/management/ampcode/model-mappings", nil) | ||
| w := httptest.NewRecorder() | ||
| r.ServeHTTP(w, req) | ||
|
|
||
| if w.Code != http.StatusOK { | ||
| t.Fatalf("expected status %d, got %d", http.StatusOK, w.Code) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test verifies that a DELETE request with an empty body to the model mappings endpoint returns a 200 OK status. However, it doesn't verify that all model mappings were actually deleted. To make the test more robust, it should be extended to make a subsequent GET request and assert that the list of model mappings is empty.
func TestDeleteAmpModelMappings_All(t *testing.T) {
h, _ := newAmpTestHandler(t)
r := setupAmpRouter(h)
req := httptest.NewRequest(http.MethodDelete, "/v0/management/ampcode/model-mappings", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected status %d, got %d", http.StatusOK, w.Code)
}
req = httptest.NewRequest(http.MethodGet, "/v0/management/ampcode/model-mappings", nil)
w = httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("GET after DELETE failed: status %d", w.Code)
}
var resp map[string][]config.AmpModelMapping
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("failed to unmarshal: %v", err)
}
if len(resp["model-mappings"]) != 0 {
t.Errorf("expected 0 mappings after DELETE all, got %d", len(resp["model-mappings"]))
}
}
No description provided.