Skip to content

Add delete_from_s3 action#700

Open
jkmassel wants to merge 1 commit intotrunkfrom
jkmassel/add-delete-from-s3
Open

Add delete_from_s3 action#700
jkmassel wants to merge 1 commit intotrunkfrom
jkmassel/add-delete-from-s3

Conversation

@jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Feb 26, 2026

Motivation

This action supports cleaning up S3 artifacts that we upload as part of CI — for example, the XCFramework snapshots published by Automattic/wordpress-rs#1199. Those artifacts are large and we don't want to store them indefinitely for no reason.

The prefix + older_than_days combination lets us set up periodic cleanup jobs (e.g. "delete all PR build artifacts older than 180 days") so storage doesn't grow unbounded.

Summary

  • Add a new delete_from_s3 fastlane action that complements the existing upload_to_s3 action
  • Supports single-key deletion (:key) and bulk deletion by prefix (:prefix)
  • Age-based filtering via :older_than_days — only delete objects older than N days
  • :dry_run mode to preview what would be deleted without making changes
  • Verbose per-object logging by default, with a :silent flag to suppress it
  • 29 RSpec tests covering both modes, pagination, batch splitting, error handling, silent mode, and parameter validation

Test plan

  • bundle exec rspec spec/delete_from_s3_spec.rb — all 29 examples pass
  • bundle exec rubocop lib/fastlane/plugin/wpmreleasetoolkit/actions/common/delete_from_s3.rb spec/delete_from_s3_spec.rb — 0 offenses
  • Review CHANGELOG entry under ## Trunk / ### New Features

🤖 Generated with Claude Code

@dangermattic
Copy link
Collaborator

dangermattic commented Feb 26, 2026

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

Complement the existing `upload_to_s3` action with a `delete_from_s3`
action that supports single-key deletion, bulk deletion by prefix,
age-based filtering (`older_than_days`), and a `dry_run` preview mode.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jkmassel jkmassel force-pushed the jkmassel/add-delete-from-s3 branch from 13d22e3 to 40c8f3d Compare February 26, 2026 19:20
@jkmassel jkmassel self-assigned this Feb 26, 2026
@jkmassel jkmassel added the enhancement New feature or request label Feb 26, 2026
@jkmassel jkmassel marked this pull request as ready for review February 26, 2026 19:21
@AliSoftware
Copy link
Contributor

Those artifacts are large and we don't want to store them indefinitely for no reason.

That's a great goal!

The prefix + older_than_days combination lets us set up periodic cleanup jobs (e.g. "delete all PR build artifacts older than 180 days") so storage doesn't grow unbounded

Got a question: what about setting up a Lifecycle rule on those paths in the S3 bucket instead, to let S3 do the cleanup for us instead of having to set up periodic cleanup jobs ourselves?

@jkmassel
Copy link
Contributor Author

what about setting up a Lifecycle rule on those paths in the S3 bucket instead

I'm generally not a fan of this for setups like ours because I'd want each repo to be able to decide how long it stores stuff for. If I want to be more aggressive and drop it down to 90 days, IMHO I shouldn't have to go touch another repo to do it.

It'd work, it just seems like it's mixing concerns across repos IMHO?

@AliSoftware
Copy link
Contributor

If I want to be more aggressive and drop it down to 90 days, IMHO I shouldn't have to go touch another repo to do it.

Yeah, I see your point.

Counter argument though: what if you want to run the cleanup cron job more frequently? You'd still have to go touch buildkite-ci repo to adjust the cronline of the buildkite_pipeline_schedule anyway 😛

It'd work, it just seems like it's mixing concerns across repos IMHO?

Depends on your perspective I guess. Personally I just think that this approach is not the right solution to reach for to solve the problem at hand.

I actually think that having to implement a cleanup of our S3 infra in your product's own repo with Fastlane + a scheduled cron job… is what would be mixing concerns across repos IMHO.

To me this is an S3 and cost management concern (something we Apps Infra are the caretakers of). Technically for most product teams we don't expect them to have to think about S3 storage and storage costs and infrastructure impacts. So this is an infra concern, and should be transparent to product teams as we don't expect them to have to manage this or have to think about it, we do. So they shouldn't have to handle the burden of setting up a scheduled job (which will require to touch another repo to begin with anyway) then create a lane to handle and implement the logic determining what to clean up and when. Having to do that on their end would be mixing concerns IMHO.

Besides, S3 Lifecycles are build explicitly for this, and have already all the logic built into it (including logic like allowing to keep only N versions of a file rather than / in addition to specifying a number of days, not requiring CI schedule jobs, etc), so creating an action for that then setting up a scheduled job etc feels like reinventing the wheel ourselves, instead of using a built-in feature of S3 that is made exactly for this at no extra cost and without requiring manual maintenance?

@iangmaia
Copy link
Contributor

You'd still have to go touch buildkite-ci repo to adjust the cronline of the buildkite_pipeline_schedule anyway

implement a cleanup of our S3 infra in your product's own repo [...] is what would be mixing concerns across repos IMHO.

To me this is an S3 and cost management concern (something we Apps Infra are the caretakers of).

Besides, S3 Lifecycles are build explicitly for this

I tend to agree, not much more to add -- I'd rather lean into a (almost) zero-maintenance, zero-code solution that's already battle tested... It's also less "operational surface area". A cron job + Fastlane action can silently break (expired credentials, CI error, etc). Lifecycle rules are declarative and managed by AWS.

Aside from the cache use case, perhaps there is another one that'd make it worth to keep the action though? 🤔 (but then at this point we're looking for a problem with a given a solution 😅 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants