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

docs(samples): add usage samples to show handling of LRO response Operation #519

Merged
merged 44 commits into from
Feb 17, 2022

Conversation

Shabirmean
Copy link
Member

@Shabirmean Shabirmean commented Feb 9, 2022

Fixes #415

Description

  • For a detailed description read internal ref b/189985582
  • The Kubernetes engine API protos were written before the API Improvement Proposal (AIP-151)
  • AIP-151 suggests to annotate API calls that takes a long time to complete with a certain annotation. This will enable the auto-generator (that generates our client libraries) to wrap the response an object of type google.longrunning.Operation in a language native (Python Future or Nodejs Promise) async construct.
  • However, since AIP-151 came long after the container.v1 API protos were written, the container API protos don't have the necessary annotation as guided in AIP-151
  • As a result the auto generated container client libraries do not return the very useful google.longrunning.Operation object, instead they return google.container.v1.Operation
  • This object does not have the necessary built-ins to easily verify if the operation has completed.
  • This this PR adds a few samples to show to customers how they can use this Operation object to validate if their operation has successfully completed using polling with backoff on the getOperation() API

Changes

  • Added 2 samples (create_cluster & delete_cluster) to showcase usage of the library and to show how to handle responses of type google.container.v1.Operation
  • Upon creation we use a fibonacci backoff to retry until the operation is complete
  • I have also added test cases for the added samples

Testing

  • Create a project
  • Enable the container (GKE) API
  • Create a service account with permissions to create/delete clusters (can use Owner)
  • Download a key for the created service account
export GOOGLE_APPLICATION_CREDENTIALS="<PATH_TO_KEY>"
gcloud config set project <GCP_PROJECT>

cd <repo_path>
git checkout lro-fix

node samples/create_cluster.js --zone=us-central1-b --name=gke10
node samples/delete_cluster.js --zone=us-central1-b --name=gke10

npm run samples-test

@Shabirmean Shabirmean requested review from a team as code owners February 9, 2022 18:56
@product-auto-label product-auto-label bot added api: container Issues related to the googleapis/nodejs-cloud-container API. samples Issues that are directly related to samples. labels Feb 9, 2022
@Shabirmean Shabirmean requested a review from sofisl February 9, 2022 18:57
@Shabirmean
Copy link
Member Author

@sofisl - Please TAL and let me know if it's good!

__Usage:__


`node samples/create_cluster.js`
Copy link
Member Author

Choose a reason for hiding this comment

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

This autogenerated usage is not correct. Im not sure how I can change this. In any case it doesn't break anything. The user will get a message indicating required args on first run

@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

@snippet-bot
Copy link

snippet-bot bot commented Feb 10, 2022

Here is the summary of changes.

You are about to add 3 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@NimJay
Copy link

NimJay commented Feb 14, 2022

Thanks for the very detailed pull-request description, Shabir. 👏

I have yet to provide more rigorous feedback and test the two scripts.
I'll likely be making some tweaks to this pull-request some time this week.

In the meantime, I realized that one of the tests is failing on Kokoro.
I believe it's because Node.js 12 doesn't support Crypto.randomUUID().
Judging from this article and this StackOverflow post, we may need to:

npm install uuid

If we could somehow use Node.js 14.17 or higher, we could use crypto.randomUUID natively. But I assume a Node upgrade is outside the scope of this PR.

@Shabirmean Shabirmean added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 16, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 16, 2022
@Shabirmean Shabirmean added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 16, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 16, 2022
Copy link
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

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

Since all the create and delete files are so similar, all of my comments for create apply to delete as well.

samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/create_cluster.js Outdated Show resolved Hide resolved
samples/system-test/test_util.js Show resolved Hide resolved
@Shabirmean Shabirmean requested a review from sofisl February 17, 2022 01:36
@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 17, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 17, 2022
@Shabirmean Shabirmean merged commit 7e4c76e into main Feb 17, 2022
@Shabirmean Shabirmean deleted the lro-fix branch February 17, 2022 20:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: container Issues related to the googleapis/nodejs-cloud-container API. samples Issues that are directly related to samples.
Projects
None yet
6 participants