Skip to content
This repository was archived by the owner on Jul 13, 2023. It is now read-only.

Call for creating an image from a disk#44

Merged
stephenplusplus merged 18 commits intogoogleapis:masterfrom
cbastuck:add_functionality_for_creating_images
Mar 16, 2018
Merged

Call for creating an image from a disk#44
stephenplusplus merged 18 commits intogoogleapis:masterfrom
cbastuck:add_functionality_for_creating_images

Conversation

@cbastuck
Copy link
Copy Markdown
Contributor

@cbastuck cbastuck commented Feb 8, 2018

Probably the function fits better in a different scope? On the other hand the path '/global/images' matches in project scope intrinsically.

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)
It's probably quicker to start a discussion from this PR?

  • [(*)] Tests and linter pass
    no new issues introduced, already on master: 6 problems (6 errors, 0 warnings)
  • Code coverage does not decrease (if any source code was changed)
    no tests have yet been added
  • [*] Appropriate docs were updated (if necessary)

Probably the function fits better in a different scope?
On the other hand the path '/global/images' matches in project scope
intrinsically.
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@googlebot 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 Feb 8, 2018
@lukesneeringer lukesneeringer added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@cbastuck
Copy link
Copy Markdown
Contributor Author

cbastuck commented Feb 8, 2018

CLA was signed

@googlebot googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@stephenplusplus
Copy link
Copy Markdown
Contributor

Thanks for the PR. I think it should definitely be scoped where you put it (on Project), although I could see it being helpful on Disk as well. What do you think?

Comment thread src/project.js Outdated
uri: '/global/images',
json: body,
},
function(err, resp) {

This comment was marked as spam.

This comment was marked as spam.

Comment thread src/project.js Outdated
* //-
*/
Project.prototype.createImage = function(imageName, disk, options, callback) {
var body = extend(

This comment was marked as spam.

This comment was marked as spam.

Comment thread src/project.js Outdated

/**
* Create a image from disk in project.
*

This comment was marked as spam.

This comment was marked as spam.

Comment thread src/project.js Outdated
*
* @param {string} imageName - Name of the target image.
* @param {Disk} disk - The source disk to create the image from.
* @param {object} options - Further options, for details please refer to

This comment was marked as spam.

This comment was marked as spam.

@cbastuck
Copy link
Copy Markdown
Contributor Author

cbastuck commented Feb 8, 2018

Thanks for the quick reply and your suggestions.

Having the call as well on Disk as well sounds good. On the other hand the disk relates to a Zone, which contradicts the 'global' concept. Thus, maybe better leave it as is, to avoid ambiguity?

@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@googlebot googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 8, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #44   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     19    +1     
  Lines        1526   1583   +57     
=====================================
+ Hits         1526   1583   +57
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/image.js 100% <100%> (ø)

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 ed0abc3...a34ef9f. Read the comment docs.

@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@googlebot googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2018
@stephenplusplus
Copy link
Copy Markdown
Contributor

Thus, maybe better leave it as is, to avoid ambiguity?

👍 SGTM

@stephenplusplus stephenplusplus added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2018
@cbastuck cbastuck force-pushed the add_functionality_for_creating_images branch from 3ae2646 to 7e2a765 Compare February 9, 2018 14:11
@googlebot googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2018
@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2018
@googlebot googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2018
@cbastuck
Copy link
Copy Markdown
Contributor Author

cbastuck commented Feb 9, 2018

Looks like node4 is a bit flaky on CI for 'Compute' test case?
I did not investigate further but noticed the node failed before this patch (same timeout reported)

@stephenplusplus
Copy link
Copy Markdown
Contributor

So, I made another change and moved the new method from Project to Compute. (Sorry, it's been a while since I've worked with this API, so I've been a little slow to recall our conventions).

Most of our methods create or return a custom class, such as Disk. We generally expose create{Object}, get{Object}s, and then the custom class has methods to modify it: {Object}.delete(), {Object}.setMetadata(), etc.

What we should really do is create the Image class that has methods that match the upstream API, like image.deprecate(), image.getMetadata(), image.setLabels(), etc.

I'm okay with not implementing those for the time being. However, before I merge, would you be interested in creating them? image.js file would basically be a clone of any of our other files, such as project.js. Then the accessor methods, like compute.getImages(), and compute.image() would be clones of compute.getFirewalls() and compute.firewall() for example.

Let me know what you think!

@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2018
@cbastuck
Copy link
Copy Markdown
Contributor Author

I signed it!

@cbastuck
Copy link
Copy Markdown
Contributor Author

Creating the Image class makes totally sense. I will look into it

@cbastuck cbastuck force-pushed the add_functionality_for_creating_images branch from e97ab2c to e9e7a72 Compare February 14, 2018 10:47
Some functions on Image concept are still missing in Image custom class
@cbastuck cbastuck force-pushed the add_functionality_for_creating_images branch from e9e7a72 to f793469 Compare February 14, 2018 10:51
@stephenplusplus
Copy link
Copy Markdown
Contributor

Hey @cbastuck, thanks for doing all of this! Sorry I couldn't review sooner. I think it looks good. There might be a few trivial, convention things I'll take care of. I'll try to do this within the next day or so, and we can release right after.

@cbastuck
Copy link
Copy Markdown
Contributor Author

Hi @stephenplusplus sounds good. Do I have to do something regarding the CLA? Maybe re-authoring all commits with the email address I signed the CLA with? When signing I added my github username which links to the 'authored' address: mail@cbastuck.de. Probably this is not resolved?

@stephenplusplus
Copy link
Copy Markdown
Contributor

Actually, I think everything is fine with the bot. It has a known bug where it gets confused when a PR has two authors. Since you were fine before I pushed my commit, I believe we're good. (@alexander-fenster can you confirm?)

@alexander-fenster
Copy link
Copy Markdown
Contributor

Could you please add tests to improve the coverage?

@alexander-fenster
Copy link
Copy Markdown
Contributor

As for the bot, everything is OK as everyone have signed the CLA and as long as all parties are OK with the changes from all parties we are good to go.

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.

So if all authors sign off the PR we can merge.

@alexander-fenster alexander-fenster removed the cla: no This human has *not* signed the Contributor License Agreement. label Feb 20, 2018
@cbastuck
Copy link
Copy Markdown
Contributor Author

I will add tests

@cbastuck
Copy link
Copy Markdown
Contributor Author

I was completely blocked be the daily work and did not find the time to increase the test coverage as promised, sorry.

@stephenplusplus stephenplusplus merged commit 44d9d8e into googleapis:master Mar 16, 2018
@ghost ghost removed the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2018
@stephenplusplus stephenplusplus mentioned this pull request Mar 16, 2018
@stephenplusplus
Copy link
Copy Markdown
Contributor

stephenplusplus commented Mar 16, 2018

No problem! Thanks for your work on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants