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

feat: add VM.get/setLabels() methods #515

Merged
merged 6 commits into from
Nov 4, 2020

Conversation

jpatokal
Copy link
Contributor

@jpatokal jpatokal commented Nov 2, 2020

feat: add VM.get/setLabels() methods

Implemented using VM.get/setTags() as a template.

Fixes #116

@jpatokal jpatokal requested a review from a team as a code owner November 2, 2020 10:13
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 2, 2020
@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #515 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          22       22           
  Lines       11861    11949   +88     
=======================================
+ Hits        11815    11903   +88     
  Misses         46       46           
Impacted Files Coverage Δ
src/vm.js 99.60% <100.00%> (+0.03%) ⬆️

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 41bd314...a6088ad. Read the comment docs.

@jpatokal jpatokal changed the title Add VM.get/setLabels() methods feat: add VM.get/setLabels() methods Nov 2, 2020
@stephenplusplus
Copy link
Contributor

stephenplusplus commented Nov 2, 2020

Thank you very much! The only thing we need to merge is a system test to confirm it works when run against the real backend. Would you be willing to add those to "system-test/compute.js"? You'd have to test against your own project, then you can run them with npm run system-test from the root directory.

Specifically, you would probably add a new "describe" block to the file, and add something like:

// using "describe.only" will only run these new tests when you test with `npm run system-test`
// (you don't want to create a bunch of extra resources that the rest of our test blocks do)

describe.only('labels', () => {
  it('should get labels', async () => {
    const [labels] = await vm.getLabels();
    assert(labels.length > 0);
  });

  it('should set labels', async () => {
    const [operation] = await vm.setLabels({myLabel: true});
    await operation.promise();
    const [labels] = await vm.getLabels();
    assert.strictEqual(labels.myLabel, true);
  });
});

No worries if not, just let me know!

Thanks again for helping out 👍🏻

Copy link
Contributor

@stephenplusplus stephenplusplus left a comment

Choose a reason for hiding this comment

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

Perfect! Just need system tests before we can merge.

@jpatokal
Copy link
Contributor Author

jpatokal commented Nov 3, 2020

System tests added. Instead of adding a new describe block, I placed it in the existing vm block, so we don't need to spin up a new VM just for this.

Note that the setLabels method requires explicitly passing in the label fingerprint. IMHO this is a little unintuitive, but it parallels the underlying API and the existing setTags method as well.

@product-auto-label product-auto-label bot added the api: compute Issues related to the googleapis/nodejs-compute API. label Nov 3, 2020
@stephenplusplus
Copy link
Contributor

Looks perfect, thank you!

@stephenplusplus stephenplusplus added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2020
@stephenplusplus stephenplusplus merged commit 60c87e6 into googleapis:master Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: compute Issues related to the googleapis/nodejs-compute API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setLabels method feature request
4 participants