fix: force setMetadata calls to use promise version, invoke callbacks manually#2000
Conversation
product-auto-label
Bot
added
size: m
ddelgrosso1
added
the
owlbot:run
gcf-owl-bot
Bot
removed
the
owlbot:run
shaffeeullah
left a comment
shaffeeullah
left a comment
There was a problem hiding this comment.
Can you also add a description to this PR to explain why this change is needed? :)
| this.setMetadata({lifecycle: {rule: newLifecycleRules}}, callback); | ||
| this.storage.retryOptions.autoRetry = this.instanceRetryValue; | ||
| this.setMetadata({lifecycle: {rule: newLifecycleRules}}) | ||
| .then(resp => callback!(null, resp)) |
There was a problem hiding this comment.
will callbacks always be provided? (guessing yes, but just wanted to double check this)
There was a problem hiding this comment.
In some situations there is no guarantee, in those places I have added a check and utilize util.noop as the callback.
| }); | ||
| }); | ||
|
|
||
| it('should execute callback with error & API response', done => { |
There was a problem hiding this comment.
For future reference, spoke with @ddelgrosso1 and determined that this case does not exist and that it would be safe to remove this test.
ddelgrosso1
added
the
do not merge
|
Marking do not merge until I fix this up and run a bunch of tests to make sure this doesn't break current behavior. |
ddelgrosso1
added
the
owlbot:run
gcf-owl-bot
Bot
removed
the
owlbot:run
|
Been running some tests locally and from what I can tell things look the same as they were. @danielbankhead would you mind giving this PR a look as well? |
danielbankhead
left a comment
danielbankhead
left a comment
There was a problem hiding this comment.
LGTM; maybe in a future refactor we can isolate the method-level retry count from the instance's retry count (perhaps via an optional param or property)
ddelgrosso1
removed
do not merge
3 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕
Rationale for this change:
setMetadatais a conditionally idempotent operation. Currently if no precondition is provided we flip a flag to turn off retries. However, sincesetMetadatais promisified we are seeing behavior where the flag is getting flipped back before the operation completes. This is causing incorrect behavior. I am changing the internal calls to wait for the promise to finish before flipping the flag back.