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.

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

[BREAKING(functions)] Update post-deploy function logic to stop aggressively deleting conatiners #8324

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

taeold
Copy link
Contributor

@taeold taeold commented Mar 16, 2025

Today, CLI attempts to clean up GCF artifacts (container images) in Artifact Registry post function deploys.

Historically, this post-deploy cleanup task had not been too reliable, and the resulting warning logs have confused developers.

We are trying a different approach - instead of trying to clean up the artifacts on every deploy, we'll make it easy for developers to set up a Artifact Registry Cleanup Policy on repository used by Cloud Run functions.

Cleanup policies are a managed service feature that automatically removes container images based on configurable rules, providing a more robust and transparent solution.

As suggested in this change, we'll automatically set up a clean up policy post-function deployment if we detect that a clean up policy doesn't exist yet.

We'll soon be updating the documentations to clarify what developers should do in order to avoid small monthly cost for Firebase Function's use of Artifact Registry.

Fixes #4954, #4954, #8164, #4757

@taeold taeold force-pushed the dl-rm-cc branch 3 times, most recently from 5f4897b to e2d426f Compare March 16, 2025 18:20
@taeold taeold requested review from inlined and blidd-google March 17, 2025 16:01
@chalosalvador
Copy link
Member

@taeold I opened the PR #8318 which may be related to this PR. It updates the packagePath method to handle the correct names for the artifacts in GCF v1 and v2 (#8164)

@taeold
Copy link
Contributor Author

taeold commented Mar 17, 2025

@chalosalvador that's a good change, but I think I'm going to suggest that Functions still move on from trying to maintain the container cleanup logic. The logic is still used in functions:deletegcfartifacts command which users can use to manually purge their Artifact Repository repo if needed, so the PR is definitely helpful.

@taeold taeold force-pushed the dl-rm-cc branch 3 times, most recently from 9a5d584 to f6cc8ac Compare March 18, 2025 02:58
@taeold taeold force-pushed the dl-rm-cc branch 3 times, most recently from 1f818f0 to a589d30 Compare March 24, 2025 20:33
@taeold taeold changed the base branch from next to master March 24, 2025 20:34
@taeold taeold requested a review from inlined March 24, 2025 20:39
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 62.79070% with 32 lines in your changes missing coverage. Please review.

Project coverage is 51.07%. Comparing base (7a7f3a5) to head (4df904f).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/deploy/functions/release/index.ts 10.00% 18 Missing ⚠️
src/deploy/functions/prompts.ts 14.28% 12 Missing ⚠️
src/functions/artifacts.ts 96.15% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8324      +/-   ##
==========================================
+ Coverage   50.94%   51.07%   +0.12%     
==========================================
  Files         425      425              
  Lines       30230    30362     +132     
  Branches     6192     6219      +27     
==========================================
+ Hits        15401    15507     +106     
- Misses      13446    13467      +21     
- Partials     1383     1388       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compromise to the internal discussion that the deployment will succeed but we'll exit with non-zero if the there is no policy, no opt-out, is non-interactive, and force isn't set.

@taeold taeold force-pushed the dl-rm-cc branch 2 times, most recently from b6e57f2 to d0145cf Compare March 25, 2025 02:10
@taeold taeold changed the title Update post-deploy function logic to stop aggressively deleting conatiners [BREAKING(functions)] Update post-deploy function logic to stop aggressively deleting conatiners Mar 25, 2025
@taeold taeold enabled auto-merge (squash) March 25, 2025 02:16
@taeold taeold merged commit 2481ece into master Mar 25, 2025
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants