-
Notifications
You must be signed in to change notification settings - Fork 925
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
Delete Container Registry images left after Functions deployment #3439
Merged
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
74d8960
Delete Container Registry images left after Functions deployment
inlined ee75643
Simplify caching
inlined dba3a0d
Improve error handling and report next steps to users
inlined b5c65ad
lint fixes
inlined 3fb4424
Merge branch 'master' into inlined.gcr-cleanup
inlined e18b3d4
Fix typo
inlined fb5a5d7
Merge branch 'inlined.gcr-cleanup' of github.com:firebase/firebase-to…
inlined b245331
Merge branch 'master' into inlined.gcr-cleanup
inlined 8b9b383
Merge branch 'master' into inlined.gcr-cleanup
inlined 72b4267
Merge branch 'master' into inlined.gcr-cleanup
inlined File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Simplify caching
- Loading branch information
commit ee75643a443b187f49e5359e560ac4fdab093810
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need to delete all tags before starting to delete the images? If not, we could combine these await Promise.all()'s and do them in parallel
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'm also a little concerned about how this behaves if just one call fails - for example, if the first Promise.all rejects, this errors and we never try to deleteImages... but we already made our recursive call, so that has started but will get 'cut off' at some arbitrary point.
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.
rm
recurse as far as it can irrespective of errors and throw a random error it encounters. Rather than investing in a multi-error type to aggregate FirebaseErrors, I'm just debug logging the error for future reference.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.
Sounds good to me - this is definitely a tricky one to handle and it seems likely that if one call fails, the rest are high likelihood to as well. I feel better about this strategy tho.