Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add functionality for passing preconditions at the function level #1993

Merged
merged 70 commits into from
Aug 10, 2022

Conversation

shaffeeullah
Copy link
Contributor

@shaffeeullah shaffeeullah commented Jun 27, 2022

Adds functionality to pass preconditions at the function level. Also includes a few fixes such as:

  • ensuring non-idempotent functions don't retry
  • applying preconditions to the correct object if there are two objects involved
    Internal ticket: b/239141167

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Jun 27, 2022
@shaffeeullah shaffeeullah added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed api: storage Issues related to the googleapis/nodejs-storage API. labels Jun 27, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Jun 28, 2022
@shaffeeullah shaffeeullah requested a review from frankyn August 5, 2022 16:29
@shaffeeullah shaffeeullah added owlbot:run Add this label to trigger the Owlbot post processor. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 5, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 5, 2022
@shaffeeullah shaffeeullah added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2022
Copy link
Contributor

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

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

Looks good, just 2 things

if (err) {
if ([408, 429, 500, 502, 503, 504].indexOf(err.code!) !== -1) {
return true;
}

if (typeof err.code === 'string') {
const reason = (err.code as string).toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

The cast here shouldn’t be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this line it throws errors: Property 'toLowerCase' does not exist on type 'never'.

test/bucket.ts Outdated Show resolved Hide resolved
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Two comments, IIUC you're extending existing options with PreconditionOptions for all operations.

src/iam.ts Outdated Show resolved Hide resolved
src/iam.ts Outdated Show resolved Hide resolved
ddelgrosso1 and others added 4 commits August 5, 2022 13:54
…#2020)

* tests: remove callback waterfall from make bucket private system test

* cleaner implementation
…pis/nodejs-storage into shaffeeullah/preconditionUpdates
@shaffeeullah shaffeeullah added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
@shaffeeullah
Copy link
Contributor Author

@frankyn can you please take another look at this when you have a sec?

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

I had one more question, otherwise LGTM.

@@ -875,6 +875,7 @@ export class Util {

if (typeof reqOpts.maxRetries === 'number') {
options.retries = reqOpts.maxRetries;
options.noResponseRetries = reqOpts.maxRetries;
Copy link
Member

Choose a reason for hiding this comment

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

naive question, shouldn't this value come from maxRetryValue instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reqOpts.maxRetries overrides maxRetryValue. It's used in cases where we need to override the user (like forcing no retries because of idempotency)

@shaffeeullah shaffeeullah requested a review from frankyn August 10, 2022 00:09
@shaffeeullah shaffeeullah merged commit 21f173e into main Aug 10, 2022
@shaffeeullah shaffeeullah deleted the shaffeeullah/preconditionUpdates branch August 10, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants