build-tools icon indicating copy to clipboard operation
build-tools copied to clipboard

feat: 'e clean' for removing artifacts

Open samuelmaddock opened this issue 1 year ago • 3 comments
trafficstars

Breaking this out as I think this warrants more discussion.

https://github.com/electron/build-tools/pull/679 introduces an artifacts/ directory for downloading pull request artifacts. To mitigate this directory growing indefinitely, I'm proposing e clean and e clean --stale.

  • e clean will remove the artifacts/ directory
  • e clean --stale removes only top-level stale files in artifacts/ (older than one month)

So folks are aware of these commands existence—and that stale files exist—we can display a warning.

$ e pr download-dist 44653
Stale artifact(s) found:
        pr_44653_darwin_arm64

Run 'e clean --stale' to cleanup artifacts.

This presents the question of how often and when to display this warning.

A. Warn about stale files each time e pr download-dist is run

  • Check for stale files and log warning

B. Warn about stale files warning on e startup

  • Every time build-tools is invoked, we check for stale files and report on them
  • To prevent spam, only warn once a week.
  • localStorage API for caching state

Maybe this all too complex and we shouldn't do this at all? Open to feedback.

samuelmaddock avatar Nov 16 '24 20:11 samuelmaddock

@MarshallOfSound Agreed on e clean not feeling quite right. It was mostly chosen to avoid analysis paralysis.

I think I was considering artifacts/ to be something broader than only PR downloads. However, maybe I should limit the scope initially. How about this:

  • e pr download-dist as it exists
    • Logs notice about stale dist artifacts
  • e pr clean-dist removes all dist artifacts
  • e pr clean-dist --stale removes all stale dist artifacts

If we come up with another use case for artifact files, we could consider moving to a top-level clean command of some sort.

Regarding pr vs ci. I guess my mind goes to "I want to download this PR's artifacts" rather than "I want to download this CI's artifacts"

samuelmaddock avatar Nov 17 '24 05:11 samuelmaddock

@MarshallOfSound do you have a preference on whether or not to merge first or to find a new name first? :slightly_smiling_face:

ckerr avatar Nov 19 '24 15:11 ckerr

@samuelmaddock I like the suggestion for e pr download-dist, e pr clean-dist and e pr clean-dist --stale. I do think there's some debating we could do about e pr vs e ci, but I personally wouldn't block over it. If you want to implement that, I think we could merge this and iterate later if we wanted to 😄

VerteDinde avatar Nov 25 '24 17:11 VerteDinde