-
Notifications
You must be signed in to change notification settings - Fork 113
Implement create instance from instance template #500
Implement create instance from instance template #500
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
=======================================
Coverage 99.61% 99.61%
=======================================
Files 22 22
Lines 11840 11846 +6
=======================================
+ Hits 11794 11800 +6
Misses 46 46
Continue to review full report at Codecov.
|
Hi @Alw3ys, would you mind adding tests for this method and signing a CLA? @stephenplusplus, would you mind taking a look at this as well? Thanks in advance! |
src/zone.js
Outdated
@@ -593,6 +594,10 @@ class Zone extends common.ServiceObject { | |||
}, | |||
config | |||
); | |||
if (body.sourceInstanceTemplate) { |
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.
Do you like the idea of mapping this to config.template
? The createVM()
method is trying to be very simple with the terminology, since the Compute API is so complex. I don't think there's another "template" concept the API would expect in this context, so the short hand should be easier to remember and intuitive from this layer.
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'm quite open for other suggestions, The reason I've chosen this way of going forward is since this is how I've seen it in other parts of this repository, and for the sake of keeping consistency, feels quite natural on my eyes to go this way.
Also I would expect something like this to pass the template id, but once again feel free to change it to something that you feel would be a better approach.
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.
Just to make sure I was being clear, I do think we should support the user passing a template reference for the request, I just thought template
would be an easier name on the user to remember/type than sourceInstanceTemplate
. As an example of similar shortcuts, config.http
expands to {tags: {items: ['http-server']}}
. Do you still think the full name the API recognizes is better? (I'm not convinced either way, just honestly interested)
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.
Oh, I didn't understand it properly.
In this case, yes. I do agree with you. I'll push the changes with what you have mentioned :)
Thank you! It looks perfect to me. I just added a test, but can't push directly. Here's a diff you can apply directly: $ curl "https://gist.githubusercontent.com/stephenplusplus/6d911fdfdadf904f329c590fc2fb2fdd/raw/8de2d6b201bed3803d21389c5a227661102f3025/compute-pr-500.diff" | git apply |
Thanks again! 🥂 |
Released under 2.2.0 👍🏻 |
Implement missing creating of vm instance from template
Fixes #114 🦕