-
Notifications
You must be signed in to change notification settings - Fork 5
Support resizing services #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 415b3a9.
Co-authored-by: Nathan Cochran <[email protected]> Signed-off-by: Smitty <[email protected]>
…o avoid token waste
| if resp.StatusCode() != 200 { | ||
| if resp.StatusCode() != http.StatusOK { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated most of our status codes to use the constants from the http package, instead of hardcoded integer literals. This is cleaner and just generally more idiomatic imo.
| if resp.JSON200 == nil { | ||
| return fmt.Errorf("empty response from API") | ||
| } | ||
| services := *resp.JSON200 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were previously being inconsistent about checking whether the response struct was nil before dereferencing it. While I think it's highly unlikely for this to happen, I decided to add the nil checks everywhere just to be safe and consistent.
| switch resp.StatusCode() { | ||
| case 202: | ||
| // Success - service creation accepted | ||
| if resp.JSON202 == nil { | ||
| fmt.Fprintln(statusOutput, "✅ Service creation request accepted!") | ||
| return nil | ||
| } | ||
| if resp.StatusCode() != http.StatusAccepted { | ||
| return common.ExitWithErrorFromStatusCode(resp.StatusCode(), resp.JSON4XX) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unnested the logic in this switch statement, since the only other case besides 202 was the default case, which just returns an error. This is how all of the other similar commands are structured.
| func ValidateAndNormalizeCPUMemory(cpuMillis, memoryGBs string) (*string, *string, error) { | ||
| func ValidateAndNormalizeCPUMemory(cpuMillis, memoryGBs string) (*CPUMemoryConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this function to return the full CPUMemoryConfig, instead of the CPU and memory strings. That makes it easier to format the resulting CPU/memory values in a variety of different ways without having to re-implement the parsing and formatting logic in different places.
| if done, err := args.Handler.InitialCheck(); done { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized the polling logic would previously always wait 1 second, poll for the service status, and then check to see whether the service was ready and it could stop polling/waiting. This worked fine for operations that pretty much always take longer than 1 second (like service create, fork, start, and stop), but resize operations sometimes happen more-or-less immediately. It's therefore beneficial to have an immediate check to see whether we even need to start polling in the first place, which is why I added this. Note that the initial check has to be a little different than the polling checks, since we don't have the full API response and status codes to go off of - just the initial service object passed in.
| // ServiceResizeOutput represents output for service_resize | ||
| type ServiceResizeOutput struct { | ||
| Status string `json:"status" jsonschema:"Current service status after resize operation"` | ||
| Resources *ResourceInfo `json:"resources,omitempty"` | ||
| Message string `json:"message"` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to make this tool only return the updated service status and the new CPU/memory values, rather than the full service details. This is similar to how the service_start and service_stop tools only return the updated status. This helps save tokens. If the LLM wants the full service details, it can call service_get.
This PR adds the ability to resize services via a new
tiger service resizecommand as well as a newservice_resizeMCP tool. Free services can be resized to paid services, but not vice-versa (downgrading a paid service to a free service is blocked via the API and results in an error).I also cleaned up some code and made some other small changes to improve how things work. See my comments below for more info on those changes.
Closes AGE-248.
Note that this PR was originally started by @syvb (see #111), but I took over and finished it after he left.