🔧 feat: Enhance config retrieval with improved error handling and diff#1931
🔧 feat: Enhance config retrieval with improved error handling and diff#1931
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the config retrieval functionality by extracting a reusable APICall function and enhancing the GetConfigFromAPI function with improved error handling and diff capabilities. The changes enable better debugging through MD5-based change detection and JSON diffing of configuration updates.
Key Changes
- Extracted
APICallas a generic reusable function for making Egg Inc API calls - Added MD5 comparison to avoid unnecessary file writes when config hasn't changed
- Integrated jsondiff library to display configuration differences for debugging purposes
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/ei/ei_api.go | Refactored GetConfigFromAPI function and extracted APICall as a reusable function; added MD5-based change detection and JSON diff output for config changes |
| go.mod | Added jsondiff v0.7.0 dependency and updated google.golang.org/genai from v1.38.0 to v1.39.0 |
| go.sum | Updated checksums for new jsondiff dependency and its transitive dependencies (tidwall packages), plus updated versions for golang.org/x/exp, golang.org/x/mod, golang.org/x/tools, and google.golang.org/genai |
| jsonData, err := json.MarshalIndent(configResponse, "", " ") | ||
| // Files are different, if we have an existing file, I want a diff | ||
| pod := []byte{} | ||
| if existingData, readErr := os.ReadFile("ttbb-data/ei-config.json"); readErr == nil { | ||
| pod = existingData | ||
| } | ||
| if patch, perr := jsondiff.Compare(pod, jsonData); perr == nil { | ||
| if b, merr := json.MarshalIndent(patch, "", " "); merr == nil { | ||
| _, _ = os.Stdout.Write(b) | ||
| } else { | ||
| log.Printf("Failed to marshal config diff; proceeding to write file: %v", merr) | ||
| } | ||
| } else { | ||
| log.Printf("Config diff failed; proceeding to write file: %v", perr) | ||
| } | ||
|
|
||
| defer func() { | ||
| if err := response.Body.Close(); err != nil { | ||
| // Handle the error appropriately, e.g., logging or taking corrective actions | ||
| log.Printf("Failed to close: %v", err) | ||
| _ = os.MkdirAll("ttbb-data", os.ModePerm) | ||
| if err = os.WriteFile("ttbb-data/ei-config.json", []byte(jsonData), 0644); err != nil { |
There was a problem hiding this comment.
The err variable is declared here but is also used later at line 378 without being reassigned. If the marshaling on line 361 fails, err will be non-nil, but this error is never checked. Then at line 378, the err variable is reassigned from WriteFile. This could lead to confusion about which operation actually failed if an error is logged at line 379. Each error should be checked immediately after the operation that produces it.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| response := APICall(reqURL, &getConfigRequest) | ||
|
|
||
| reqBin, err := proto.Marshal(&getConfigRequest) | ||
| if err != nil { | ||
| log.Print(err) | ||
| return false | ||
| } | ||
| values := url.Values{} | ||
| reqDataEncoded := enc.EncodeToString(reqBin) | ||
| values.Set("data", string(reqDataEncoded)) | ||
| configResponse := &ConfigResponse{} | ||
| opts := proto.UnmarshalOptions{ | ||
| DiscardUnknown: true, | ||
| } | ||
| err := opts.Unmarshal(response, configResponse) | ||
| if err != nil { | ||
| log.Print(err) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Missing nil check for the response from APICall. If APICall returns nil due to any error condition (marshaling, network, decompression, etc.), the subsequent Unmarshal will attempt to process nil data, which could lead to unexpected behavior. Add a nil check before attempting to unmarshal the response.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
| } | ||
| } | ||
| jsonData, err := json.MarshalIndent(configResponse, "", " ") |
There was a problem hiding this comment.
The jsonData variable is marshaled twice - once inside the MD5 comparison block (line 351) and again here (line 361). This is inefficient and could lead to inconsistencies if marshaling produces different results. Consider reusing the newJSONData from the comparison block or restructuring the logic to marshal only once.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@mkmccarty I've opened a new pull request, #1932, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@mkmccarty I've opened a new pull request, #1933, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.