[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Support for functions failure policies #1858

Merged
merged 8 commits into from
Aug 20, 2020
Merged

Support for functions failure policies #1858

merged 8 commits into from
Aug 20, 2020

Conversation

samtstern
Copy link
Contributor
@samtstern samtstern commented Dec 11, 2019 •
edited
Loading

Description

Fixes: #1734
Follow up to: firebase/firebase-functions#482

Depends on:
firebase/firebase-functions#760

Scenarios Tested

Testing using three functions:

const functions = require('firebase-functions');
const admin = require('firebase-admin');
admin.initializeApp();

exports.policyEmptyObject = functions
  .runWith({
    failurePolicy: { retry: {} }
  })  
  .pubsub.topic("retry-topic")
  .onPublish((msg, ctx) => {
    return true;
  });


exports.policyTrue = functions
  .runWith({
    failurePolicy: true,
  })
  .pubsub.topic("retry-topic")
  .onPublish((msg, ctx) => {
    return true;
  });

exports.policyNone = functions
  .pubsub.topic("retry-topic")
  .onPublish((msg, ctx) => {
    return true;
  });

Sample Commands

N/A

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Dec 11, 2019
@alexlouden alexlouden mentioned this pull request Jan 7, 2020
Closed
@deremer deremer mentioned this pull request Jan 7, 2020
Closed
@merlinnot merlinnot mentioned this pull request Jan 24, 2020
Closed
@rhodgkins
Copy link
Contributor

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!

@merlinnot
Copy link
Contributor

@rhodgkins This stopped at #1734 (comment), I never heard of this issue being fixed. @samtstern Could you check if it's fixed?

@samtstern
Copy link
Contributor Author

@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.

@merlinnot
Copy link
Contributor

Awesome!

@samtstern samtstern mentioned this pull request Aug 13, 2020
Merged
@samtstern
Copy link
Contributor Author
samtstern commented Aug 13, 2020 •
edited
Loading

@merlinnot I'm having some real trouble testing this ... It's been a while but I think I need to:

  1. Checkout firebase-functions and switch to the branch with failurePolicy
  2. In firebase-tools I need to npm install --save ../firebase-functions to get my local version
  3. In my Firebase functions folder (test project) I need to find a way to bundle firebase-functions so that my local dependency is uploaded at firebase deploy

(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 npm pack

@samtstern
Copy link
Contributor Author

@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.

samtstern
samtstern commented Aug 13, 2020
View reviewed changes
package.json Outdated Show resolved Hide resolved
@samtstern samtstern changed the title WIP: Support for functions failure policies Support for functions failure policies Aug 13, 2020
@samtstern samtstern marked this pull request as ready for review August 13, 2020 17:03
@samtstern samtstern requested a review from joehan August 13, 2020 17:03
@merlinnot
Copy link
Contributor

Brilliant! The other PR was already approved and merged once, so we shouldn't have much trouble with landing it 😂

joehan
joehan approved these changes Aug 13, 2020
View reviewed changes
Copy link
Contributor
@joehan joehan left a 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!

src/deploy/functions/release.js Outdated Show resolved Hide resolved
@samtstern
Copy link
Contributor Author

Thanks @joehan ! Could you also take a look at firebase/firebase-functions#760 which is a prerequisite to this PR?

@stari4ek
Copy link

BTW, there is no way to enable retries for firebase functions using GCP UI now.
When I enable "Retry on failure" for function with "Cloud Firestore" trigger - it requires to re-deploy function using web UI.
Using already deployed source code ("Inline Editor" selected) gives build error (most probably since it expects google cloud function and does not have any magic applied by firebase cli): Build failed: function.js does not exist; Error ID: 7485c5b6

So it means there is no way to enable retries at all

@samtstern
Copy link
Contributor Author

firebase-functions version 3.10.0 is out which contains this feature. Tests are passing and an end-to-end test worked on my end! Going to merge!

@samtstern samtstern merged commit e92ba5a into master Aug 20, 2020
armordog
armordog reviewed Aug 20, 2020
View reviewed changes
src/deploy/functions/release.js
utils.logLabeledWarning("functions", retryMessage);

let proceedPrompt = Promise.resolve(true);
if (options.nonInteractive && !options.force) {
Copy link

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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)

Copy link

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. =]

Copy link
Contributor Author

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 :-)

Copy link

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. =]

@bkendall bkendall deleted the ss-failure-policy branch August 4, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armordog armordog armordog left review comments

@joehan joehan joehan approved these changes

Assignees
No one assigned
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for failure policies
8 participants
@samtstern @rhodgkins @merlinnot @stari4ek @armordog @joehan @googlebot @mbleigh