-
Notifications
You must be signed in to change notification settings - Fork 950
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
fix: prevent failure when deleting cloud function with missing schedule #7787
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@taeold and @christhompsongoogle are tagged in the associated issue |
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.
Thanks for the contribution! I added some early review feedback, and assigned some folks with more context on functions to give a more thorough pass.
Could you also add a changelog entry?
if ( | ||
err instanceof reporter.DeploymentError && | ||
err.op == "delete schedule" && | ||
(err.original as FirebaseError).message == "HTTP Error: 404, Job not found." |
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.
Drive by: Probably safer to just check if err.status == 404 - relying on the exact string is vulnerable to breaking in the future.
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 considered that; however, I wasn't sure whether the backend here may re-use the 404 status code for a scenario where we would not want to continue cleanup.
I can adjust this either way based on guidance from someone familiar with the possible failure modes here.
else if ( | ||
err instanceof reporter.DeploymentError && | ||
err.op == "delete topic" && | ||
err.message.startsWith("Failed to delete topic function") |
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.
This concerns me - if the topic deletion fails for a generic reason, I don't like orphaning the resource. If we can check for specific failures (ie 404s), I'd be more comfortable with this
Co-authored-by: joehan <joehanley@google.com>
Fixes #4795 |
Closes: 4795
Description
Fixes #4795 (comment)
When deletion of a
pubsub.schedule
Cloud Function partially fails, manual interaction is required to fully delete the function.In my case, deletion of the Cloud Function was unpredictably failing after deleting the associated Cloud Schedule, leaving the function unable to be deleted by a deployment even with the
--force
CLI flagThis MR changes the behavior of Cloud Functions deployment so that a function which is supposed to have an associated schedule will be deleted if the schedule no longer exists.
Scenarios Tested
Using the latest build of
master
@a54e4ff20cc0aca11d89e2f6b31c35f063918d87
firebase deploy
a. I am unsure what specific scenarios cause the schedule to be deleted without the function being deleted, but it's happened to me a number of times
b. Notice the
Trigger
column in the Firebase console becomes blank:firebase deploy
a. Observe the failure in the deploy logs:
HTTP Error: 404, Job not found.
b. Observe the final failure notice in the deploy logs:
Using the changes from this merge request
firebase deploy
a. Observe the warning in the deploy logs:
Sample Commands