Skip to content
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

compute: vm: add resize() #1469

Merged
merged 4 commits into from
Aug 29, 2016
Merged

Conversation

stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Aug 5, 2016

This takes over from #1448, adding a resize method on the VM object.

To Dos

  • Start the VM again after setting the new machine type
  • Rename method
  • Explain the stopping and starting in the docs
vm.resize('n1-standard-1', function(err, apiResponse) {
  if (!err) {
    // VM resized successfully!
  }
});

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 5, 2016
@stephenplusplus stephenplusplus added the api: compute Issues related to the Compute Engine API. label Aug 5, 2016

callback = callback || common.util.noop;

this.stop(function(err, operation, apiResponse) {

This comment was marked as spam.

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor Author

@callmehiphop could you take a look at the updates and see if this is the best case for the user? It should definitely be the most "automatic", I'm just not sure if it could create surprises. I've played with an options object to control the behavior, but it felt too verbose and confusing. LMK if you think this new behavior is okay or if you have any ideas on how to improve.

@@ -579,4 +637,24 @@ VM.prototype.request = function(reqOpts, callback) {
});
};

VM.prototype.execAfterOperation_ = function(callback) {

This comment was marked as spam.

This comment was marked as spam.

@callmehiphop
Copy link
Contributor

I'm a bit hesitant making all these guesses, only because to the user it might feel a bit magical. What if instead of just automatically doing all this, we added an option like restart? That way it was a bit less magical and if for some reason the user wanted to programmatically restart the VM on their own, they could.

@stephenplusplus
Copy link
Contributor Author

That is an option, but makes the API have different response args to the user's callback. If we just do the API call, which assumes the VM is not running, we would return an operation. If we do a restart, what do we return? I thought it would be weird to return the vm.start() operation.

@callmehiphop
Copy link
Contributor

Perhaps setMachineType shouldn't restart the VM then? We can have it simply set the machine type and nothing else. Then add a convenience method that sits on top of it and will restart the VM?

@stephenplusplus
Copy link
Contributor Author

What would that look like? Having two methods for doing the same thing isn't very "us"-- in this case, I think we just have to pick one or the other. A machine type can only be changed if the machine isn't running, so that means if we just expose the "raw" way (just make the API call), the user will always have to call vm.stop() first. And since we're usually "smart" for the user, that's why I keep ending up at doing it for them.

Maybe we should wait for the next meeting and bring it up then.

@stephenplusplus
Copy link
Contributor Author

Feedback was to change to a name that implies magic is happening, i.e. vm.resize(). Whereas setMachineType doesn't imply "we're restarting your VM".

@stephenplusplus stephenplusplus changed the title compute: vm: add setMachineType compute: vm: add resize() Aug 29, 2016
@stephenplusplus
Copy link
Contributor Author

@callmehiphop PTAL! I renamed the method to resize, put a "private" execAfterOperation_ method on the top level Compute instance*, wrote some tests, and added docs.

*the method is not static because to use it from "VM", we introduce a cross dependency from index.js and vm.js. This didn't make the require call throw, but did result in an undefined Compute.execAfterOperation_ method. It was easier to put it on the Compute instance.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5152158 on stephenplusplus:spp--1448 into 069d827 on GoogleCloudPlatform:master.

*
* In order to change the machine type, the VM must not be running. This method
* will automatically stop the VM if it is running before changing the machine
* type. After it is sucessfully changed, the VM will be started.

This comment was marked as spam.

This comment was marked as spam.

@callmehiphop callmehiphop merged commit 737b916 into googleapis:master Aug 29, 2016
@coveralls
Copy link

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 457e882 on stephenplusplus:spp--1448 into 069d827 on GoogleCloudPlatform:master.

sofisl pushed a commit that referenced this pull request Nov 17, 2022
* compute: vm: add setMachineType

* restart if necessary

* rename to resize

* add `start` option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: compute Issues related to the Compute Engine API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants