Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added OAuth to provisioner stop method #365

Merged
merged 8 commits into from
Mar 23, 2016

Conversation

gitlaura
Copy link
Contributor

This change is Reviewable

@@ -104,8 +105,13 @@ class Provisioner {
* @param {String} name of VM to create
* @return {Promise.<Object>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's quickly describe the format of the return type while we're at it...is it just:

{
  droplet: <droplet id, as a string>
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the return type to Promise since there isn't anything to return.

@trevj
Copy link
Contributor

trevj commented Mar 18, 2016

Sorry for the delay!

@gitlaura
Copy link
Contributor Author

I updated provisioner.ts to separate the stop and destroyServer_ methods so that we can call destroyServer_ if there's a failure when creating a cloud server.

* @return {Promise.<void>}
*/
private destroyServer_ = (name: string): Promise<void> => {
if (this.state_.oauth) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than wrapping the whole function in a try/catch block you could make this a precondition-like block:

if (!this.state_.oauth) {
  return Promise.reject({
    'errcode': 'OAUTH_ERR',
    'message': 'Cannot destroy server - not logged into digital ocean'
  });
  ...
  ...
}

Actually...since this is a private method only called by stop, do you really need this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that we don't need it. Made this change.

@trevj
Copy link
Contributor

trevj commented Mar 22, 2016

BTW, am investigating the Travis failures now...

@trevj
Copy link
Contributor

trevj commented Mar 23, 2016

👍

@gitlaura gitlaura merged commit debae7b into master Mar 23, 2016
@gitlaura gitlaura deleted the gitlaura-destroy-cloud-server branch March 23, 2016 20:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants