-
Notifications
You must be signed in to change notification settings - Fork 2
Support resizing services #111
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
|
The frontend doesn't let you resize between shared and non-shared, so I didn't add shared to the list of resize options. But right now it will still let you resize a shared instance to a non-shared instance, which doesn't actually work (the API response is successful, but nothing changes). I think the right approach is to fix the backend so that it gives a proper error here. |
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.
Looks good overall! I left a bunch of comments, but they're mostly minor stylistic or structural/organizational things.
I think the biggest issues are:
- Your branch is decently far behind
mainat this point. I think you should merge inmain, resolve the conflicts, and update things to use the latest patterns. See my comments below for more info. - I think we should update the resize REST API endpoint to accept the CPU and memory values as strings instead of as integers before releasing this (for consistency with the other endpoints, and for more flexibility going forwards). If we don't do that now, it's going to be much harder to do in the future while maintaining backwards-compatibility with existing versions of the CLI. I think there are some other things we should fix with that endpoint too, like getting rid of the unused
nodesparameter. In general, we've been reviewing and improving the REST API endpoints every time we implement a new CLI command, so I think it's worth doing that in this case too. - It would be good to add some integration tests for the new resize command. See integration_test.go. You can probably just add a new subtest in TestServiceLifecycleIntegration.
Thanks for tackling this! 🙏
Co-authored-by: Nathan Cochran <nathanjcochran@gmail.com> Signed-off-by: Smitty <me@iter.ca>
c0a3723 to
1837c91
Compare
…o avoid token waste
Supports changing the CPU/RAM of instances.