-
Notifications
You must be signed in to change notification settings - Fork 959
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
Support for functions failure policies #1858
Conversation
Any ideas what's happened to this - is it still having to go through an API review or something? Would be great to see it out and released! |
@rhodgkins This stopped at #1734 (comment), I never heard of this issue being fixed. @samtstern Could you check if it's fixed? |
@merlinnot I still have not been able to get the issue with the Functions backend resolved. But there's always a chance someone fixed the backend without knowing, I'll give this another run today. |
Awesome! |
@merlinnot I'm having some real trouble testing this ... It's been a while but I think I need to:
(2) and (3) are failing in fun ways Have you ever tried to test this end-to-end? Am I making this harder on myself than it needs to be? Edit: @mbleigh has told me about the wonders of |
@merlinnot it works now! I guess the backend fixed the issue at some point, or I'm doing something differently. But once firebase/firebase-functions#760 is released we can proceed with this. |
Brilliant! The other PR was already approved and merged once, so we shouldn't have much trouble with landing it 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one question about wording. 🥳 for this finally working!
Thanks @joehan ! Could you also take a look at firebase/firebase-functions#760 which is a prerequisite to this PR? |
BTW, there is no way to enable retries for firebase functions using GCP UI now. So it means there is no way to enable retries at all |
|
utils.logLabeledWarning("functions", retryMessage); | ||
|
||
let proceedPrompt = Promise.resolve(true); | ||
if (options.nonInteractive && !options.force) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check if there are any functions with a retry policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh ... I think so! Want to send a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or I can do it, but you deserve the glory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the opportunity but I have a lot to do right now. =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armordog done and released!
https://github.com/firebase/firebase-tools/releases/tag/v8.8.1
Thank you for catching that! Saved me a lot of angry emails in the morning :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was fast!
Thanks so much. =]
Description
Fixes: #1734
Follow up to: firebase/firebase-functions#482
Depends on:
firebase/firebase-functions#760
Scenarios Tested
Testing using three functions:
Sample Commands
N/A