Skip to content

I can't believe we missed this.#647

Merged
johnml1135 merged 3 commits intomainfrom
fix_delete_files
Mar 13, 2025
Merged

I can't believe we missed this.#647
johnml1135 merged 3 commits intomainfrom
fix_delete_files

Conversation

@johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented Mar 12, 2025

This change is Reviewable

@johnml1135 johnml1135 requested a review from Enkidu93 March 12, 2025 18:54
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)


a discussion (no related file):
We are intentionally not deleting files. Deleting resources on a web API with asynchronous processes is a tricky thing. Deleting a file while it is currently being used by a build can cause unexpected behavior, so we purposefully wait to delete the file. The DeletedFileCleaner class is responsible for actually deleting the file.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.68%. Comparing base (8e7f367) to head (a168a30).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##            main     #647       +/-   ##
==========================================
+ Coverage   0.00%   64.68%   +64.68%     
==========================================
  Files        342      342               
  Lines      18704    18705        +1     
  Branches    2427     2427               
==========================================
+ Hits           0    12099    +12099     
+ Misses     18704     5729    -12975     
- Partials       0      877      +877     

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

@johnml1135
Copy link
Collaborator Author

Previously, ddaspit (Damien Daspit) wrote…

We are intentionally not deleting files. Deleting resources on a web API with asynchronous processes is a tricky thing. Deleting a file while it is currently being used by a build can cause unexpected behavior, so we purposefully wait to delete the file. The DeletedFileCleaner class is responsible for actually deleting the file.

Changed to an explanatory note. The next person will not think it's a bug then :-)

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)


a discussion (no related file):

Previously, johnml1135 (John Lambert) wrote…

Changed to an explanatory note. The next person will not think it's a bug then :-)

Are deleted files not getting cleaned up?

@johnml1135
Copy link
Collaborator Author

Previously, ddaspit (Damien Daspit) wrote…

Are deleted files not getting cleaned up?

I didn't give it 24 hours - just 12. I'll check again to make sure they actually got removed.

@johnml1135
Copy link
Collaborator Author

Previously, johnml1135 (John Lambert) wrote…

I didn't give it 24 hours - just 12. I'll check again to make sure they actually got removed.

Verified it - they did delete after 24 hours. I would still like to add this comment though, to help the next person.

@johnml1135 johnml1135 merged commit bc3e1e1 into main Mar 13, 2025
2 of 3 checks passed
@johnml1135 johnml1135 deleted the fix_delete_files branch March 13, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants