feat: add binding for instance group manager #104

toots · 2018-06-13T17:53:30Z

Fixes #103

This is missing a couple of methods and untested but should be a good start to see if that's headed to the right direction.

fhinkel · 2018-06-13T17:59:23Z

Thanks! I think the linter recently switch the format. Can you update the linter and rerun?

codecov · 2018-06-13T18:57:53Z

Codecov Report

Merging #104 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #104    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          19     21     +2     
  Lines        1581   1749   +168     
======================================
+ Hits         1581   1749   +168

Impacted Files	Coverage Δ
src/autoscaler.js	`100% <ø> (ø)`	⬆️
src/index.js	`100% <100%> (ø)`	⬆️
src/instance-template.js	`100% <100%> (ø)`
src/instance-group-manager.js	`100% <100%> (ø)`
src/zone.js	`100% <100%> (ø)`	⬆️
src/operation.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 379ef81...945b547. Read the comment docs.

toots · 2018-06-13T18:58:15Z

lint tests fixed :-)

toots · 2018-06-14T16:44:14Z

I've added bindings for removeVMs and resize. However, I hit a snag looking at setInstanceTemplate & realized instance templates aren't supported yet.

I will try to have a look at it as well. Is it customary for these APIs to pass an instance of InstanceTemplate to setInstanceTemplate and extract the URL from the object or should the user pass directly the instanceURL as set by the server?

toots · 2018-06-14T20:46:20Z

Okay, instanceTemplate is mostly there, still untested. Unless I hear back I'm gonna go ahead and enable passing Object<instanceTemplate> | String<instanceTemplateUrl> as instance template parameter for the instance group manager. This code is still totally untested but I've got an application waiting to use it so it'll eventually happen..

creation.

toots · 2018-06-15T06:51:01Z

Ok, lookin at how images are created I did the same for instance group managers, adding mandatory instanceTemplate parameter and un-documenting autoCreate. That should wrap up the functionalities for this PR. Only thing I guessed was wether or not it is possible to use relative URLs for the instance template, same way as is done with disk image in image creation. I'm gonna test this in my application now so we shall see.. :-)

encode and test as function parameters. This reverts commit 6957c62.

toots · 2018-06-15T09:19:59Z

Ok, it would appear that at least the create functions are working, thus I think this PR is ready for review!

…p-manager

stephenplusplus

All is looking very good, just some small comments so far. Thank you for your help and patience!

src/index.js

@@ -351,7 +353,8 @@ Compute.prototype.createHealthCheck = function(name, options, callback) {
 * //-
 * compute.createImage('new-image', disk).then(function(data) {
 *   var operation = data[0];
- *   var apiResponse = data[1];
+ *   var image = data[1];


src/index.js

+ *   const apiResponse = data[2];
+ * });
+ */
+Compute.prototype.createInstanceTemplate = function(name, options, callback) {


src/index.js

+ *
+ * @param {string} name - The name of the target image.
+ * @param {object=} options - See an
+ *     [InstanceTemplate resource](https://cloud.google.com/compute/docs/reference/v1/instanceTemplate#resource).


src/index.js

+  var self = this;
+
+  if (typeof options !== 'object') {
+    throw new Error('options object is required.');


src/index.js

+ * Get a list of instance templates.
+ *
+ * @see [InstanceTemplates Overview]{@link https://cloud.google.com/compute/docs/instanceTemplates}
+ * @see [InstanceTemplates: list API Documentation]{@link https://cloud.google.com/compute/docs/reference/v1/instanceTemplates}


src/zone.js

+    throw new Error('An InstanceTemplate object is required.');
+  }
+
+  if (typeof targetSize !== 'number') {


src/instance-group-manager.js

@@ -0,0 +1,550 @@
+/*!
+ * Copyright 2016 Google Inc. All Rights Reserved.


src/instance-group-manager.js

+/**
+ * Get a list of managed VM instances in this instance group manager.
+ *
+ * @see [InstaceGroups: listInstances API Documentation]{@link https://cloud.google.com/compute/docs/reference/v1/instanceGroupManagers/listInstances}


src/instance-group-manager.js

+ *   const apiResponse = data[1];
+ * });
+ */
+InstanceGroupManager.prototype.recreateVMs = function(callback) {


src/instance-group-manager.js

+  this.request(
+    {
+      method: 'POST',
+      uri: '/resize?size=' + parseInt(size),


toots · 2018-06-28T11:51:49Z

Ok, all review remarks should have been fixed now. Thanks for looking at it!

toots · 2018-09-06T16:19:54Z

Ok, sorry it took me so long to fix those last remarks. Should be all good now!

googlebot · 2018-09-12T12:09:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

stephenplusplus · 2018-09-12T12:26:28Z

⚠️ Update!

I added a commit to do a few things:

Add system tests to verify the new stuff works against the real API
Add Compute#getInstanceGroupManagers() (and the stream counterpart) for listing global instance group manager
Export InstanceGroup from the Compute class
Add InstanceGroupManager#setInstanceTemplate
Add and require VMs argument to InstanceGroupManager#removeVMs

⌚️ Status!

I'm stuck a bit, as other tasks are pushing their way in front of this one. We still need:

Unit tests for the new methods I added:
- Compute#getInstanceGroupManagers()
- Compute#getInstanceGroupManagersStream()
- InstanceGroupManager#setInstanceTemplate()
System tests for:
- InstanceTemplate functionality (creating, listing, listing as stream)
- InstanceGroupManager#recreateVMs()
- InstanceGroupManager#removeVMs()
- InstanceGroupManager#setInstanceTemplate()

I hope to get back to this soon, but if you want to play around and take on any of these, feel free!

Thanks again for your help and patience

toots · 2018-09-28T20:01:19Z

Hi folks,

I just merged the master branch and adapted to the updated syntax as much as I could. It looks like coverage tests aren't passing anymore.

I'd like to say that this process has been a tad bit frustrating. My original PR had, I believe, a lot of merit on its own to begin with.

While I appreciate the reviews and being guided through the various layers of code quality tools here, I am not part of the dev team or working for this project, just a user who wishes to contribute back.

The more this PR drags, the more difficult it will be to keep it up-to date. Updating the syntax without merging it was, in a way, pretty inconsiderate if you think about it.

I'd love to see this PR merged but, as it stands, I don't think that I am willing to contribute to it past this point.

It would be great to see this binding cover some much needed part of the upstream API, however.

Cheers,
Romain

stephenplusplus · 2018-09-29T14:32:20Z

Hey Romain, I'm frustrated for you as well. Thanks again for all the help you've provided. It's mostly a matter of attention being spread across all of our projects, and I'm sorry I couldn't spend more on this. When I was able to, seeing the small details that you nailed (docs links, types, test grouping) was very appreciated.

For now, this will probably have to sit for a bit longer, due to that same attention issue :( I hope I can come back soon, but for now, my list of "To Dos" above have to be conquered before it's shippable.

As always, thank you very much for your help, and I hope this won't deter you from contributing again if you see a mistake or just have an idea.

sduskis · 2018-12-17T16:00:19Z

@toots and @stephenplusplus, this PR is really out of date. Is this PR salvageable?

toots · 2018-12-17T16:56:54Z

Well, in a more general context, I believe that it'd be nice if the library could cover all of the API functionalities. We use these calls on one of our work project and the fact that they aren't yet part of the released/official code has forced us to work out of a patched version and prevented a full open source release of it.

I'd be willing to work on salvaging this one PR but only if there's a clear path for a merge.

JustinBeckwith · 2018-12-17T17:08:39Z

@toots I really apologize for this one - totally my fault. We do try to be a lot more attentive to PRs, but this one is honestly just bad luck. I believe this is the only repository we have that isn't green right now, and it's been broken on master for a number of months. I'm spending some time over the next few weeks trying to get us green, and get us back into a state where the library is shippable. Once we're in that state - I would be happy to re-engage, and see if we can get this landed. Until then - I want to be respectful of your time.

If you're blocked - another option here is the apiary library:
https://github.com/googleapis/google-api-nodejs-client

The apiary library is autogenerated, so the API coverage is going to be 100%. It's going to be far less polished and easy to use, but it will be complete.

Apologies again, and thanks for sticking with us here :)

toots · 2018-12-17T17:12:09Z

Sounds good thanks for getting back to me. I'll wait and look at the apiary library meanwhile.

JustinBeckwith · 2018-12-21T05:06:51Z

Ok! So we're in a better place now :)

sduskis · 2019-01-22T16:49:12Z

@toots, this PR has gone stale. Is it worth reviving it?

toots · 2019-01-22T16:52:30Z

I would love to finish it but I'm swamped at the moment. Do you guys need it finished by a certain deadline?

JustinBeckwith · 2019-01-22T18:01:39Z

Not at all :) Any help you can offer is awesome, whenever you have time (or its cool even if you don't). We really appreciate the help!

modeswitch · 2019-02-10T06:08:46Z

I'm willing to take this over so the maintainer can land it. Any objections?

modeswitch · 2019-02-10T18:58:05Z

Rather than do extensive surgery here, I created #263. It only adds basic support, but I think it will be easier to add features once the basic plumbing is landed.

JustinBeckwith · 2019-02-11T03:00:15Z

This was covered in #263 🥂

Add binding for instance group manager.

166994d

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 13, 2018

Fix lint tests.

aecb942

toots added 2 commits June 14, 2018 18:40

removeVMs.

b144383

Resize.

2d0fa70

toots added 4 commits June 14, 2018 18:53

Add missing test.

757521e

Make lint happy.

4a1f106

Getting started with instanceTemplate.

d131494

Make lint happy.

434b0ef

toots added 4 commits June 15, 2018 08:06

Move instance template creation to compute, add list.

f47163f

Fix doc.

5cf5de7

Add streamed list for instance group managers.

6dc11ef

Add instance template as mandatory parameter for instance group manager

1d58efa

creation.

toots added 3 commits June 15, 2018 10:53

Add mandatory targetSize for instance group manager.

b0a396b

Add required machineType to instance template creation.

6957c62

Required parameters for instance template creation are too complex to

f6c9e2f

encode and test as function parameters. This reverts commit 6957c62.

toots added 3 commits June 15, 2018 11:26

Make options argument mandatory for createInstanceTemplate.

69bb4fd

Merge https://github.com/googleapis/nodejs-compute into instance-grou…

c64f519

…p-manager

Fix unrelated typo.

364651f

stephenplusplus suggested changes Jun 22, 2018

View reviewed changes

toots added 2 commits June 28, 2018 13:50

Apply review changes.

8fd0d05

Merge branch 'master' into instance-group-manager

05e951b

Fix latest remarks.

12a2732

toots and others added 3 commits September 6, 2018 11:29

Make lint happy.

8013b44

Merge 8013b44 into 32143a2

e984f01

WIP

e861711

ghost assigned stephenplusplus Sep 12, 2018

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 12, 2018

stephenplusplus mentioned this pull request Sep 12, 2018

getVMs() of regionInstanceGroup #146

Closed

Merge branch 'master' into instance-group-manager

945b547

JustinBeckwith changed the title ~~Add binding for instance group manager.~~ feat: add binding for instance group manager Dec 21, 2018

yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019

JustinBeckwith added the needs work This is a pull request that needs a little love. label Feb 10, 2019

JustinBeckwith closed this Feb 11, 2019

yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020

		@@ -0,0 +1,550 @@
		/*!
		* Copyright 2016 Google Inc. All Rights Reserved.

feat: add binding for instance group manager #104

feat: add binding for instance group manager #104

Conversation

toots commented Jun 13, 2018

fhinkel commented Jun 13, 2018

codecov bot commented Jun 13, 2018 • edited Loading

Codecov Report

toots commented Jun 13, 2018

toots commented Jun 14, 2018 • edited Loading

toots commented Jun 14, 2018

toots commented Jun 15, 2018

toots commented Jun 15, 2018

stephenplusplus left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

toots commented Jun 28, 2018

toots commented Sep 6, 2018

googlebot commented Sep 12, 2018

stephenplusplus commented Sep 12, 2018

toots commented Sep 28, 2018 • edited by fhinkel Loading

stephenplusplus commented Sep 29, 2018

sduskis commented Dec 17, 2018

toots commented Dec 17, 2018

JustinBeckwith commented Dec 17, 2018

toots commented Dec 17, 2018

JustinBeckwith commented Dec 21, 2018

sduskis commented Jan 22, 2019

toots commented Jan 22, 2019

JustinBeckwith commented Jan 22, 2019

modeswitch commented Feb 10, 2019

modeswitch commented Feb 10, 2019

JustinBeckwith commented Feb 11, 2019

codecov bot commented Jun 13, 2018 •

edited

Loading

toots commented Jun 14, 2018 •

edited

Loading

toots commented Sep 28, 2018 •

edited by fhinkel

Loading