Skip to content

Save scratch assets to bucket#827

Open
zetter-rpf wants to merge 5 commits into
mainfrom
save-scratch-assets-to-bucket
Open

Save scratch assets to bucket#827
zetter-rpf wants to merge 5 commits into
mainfrom
save-scratch-assets-to-bucket

Conversation

@zetter-rpf
Copy link
Copy Markdown
Contributor

@zetter-rpf zetter-rpf commented May 21, 2026

Status

To do:

  • Decide if a caching header needs to be added
  • Decide if any CORS headers need to be added

What's changed?

This syncs Scratch asset library assets into a bucket so we can serve them over a CDN.

I intend to use the same bucket across all the environments- as objects are keyed by their MD5 hash I think this is safe to do. When updating we should run it in production.

Steps to perform before deploying to production

@cla-bot cla-bot Bot added the cla-signed label May 21, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Test coverage

91.33% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/26457258124

My next commit will add in another task to the import, having one instance of ScratchAssetImporter per asset, will make easier to cache the asset and not download it twice
@zetter-rpf zetter-rpf force-pushed the save-scratch-assets-to-bucket branch from f59f5d8 to 4108979 Compare May 21, 2026 15:35
Many assets from the asset library are rendered on the page at once which will cause a lot of requests to editor API.

This is being stored in the editor-assets r2 bucket. Note that region isn't used by has to be set to a valid value.
@zetter-rpf zetter-rpf force-pushed the save-scratch-assets-to-bucket branch from 4108979 to ea17a52 Compare May 21, 2026 16:01
@zetter-rpf zetter-rpf temporarily deployed to editor-api-p-save-scrat-omfb8e May 21, 2026 16:01 Inactive
Since these are keyed by their md5 hash we can cache them for a long time (I've chosen 7 days). R2 does add an etag, but the adding this will mean etags won't need to be checked and might enable cloudflare to do more caching.
@zetter-rpf zetter-rpf temporarily deployed to editor-api-p-save-scrat-s7dvb9 May 22, 2026 15:49 Inactive
@zetter-rpf zetter-rpf marked this pull request as ready for review May 26, 2026 13:27
Copilot AI review requested due to automatic review settings May 26, 2026 13:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the Scratch asset syncing workflow so that imported Scratch library assets can also be uploaded into an “editor assets” object bucket (intended for CDN serving), while also renaming the importer entrypoint from import to import_all.

Changes:

  • Renames ScratchAssetImporter.import to ScratchAssetImporter.import_all and updates callers/specs accordingly.
  • Adds optional S3/R2 upload of fetched assets to an editor assets bucket (with cache-control metadata).
  • Updates the rake task description to link operational usage instructions.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
spec/lib/scratch_config_importer_spec.rb Updates expectations to call ScratchAssetImporter.import_all.
spec/lib/scratch_asset_importer_spec.rb Renames spec target to import_all and adds bucket-sync test coverage.
lib/tasks/scratch_assets.rake Updates task description to include usage link.
lib/scratch_config_importer.rb Switches importer call from import to import_all.
lib/scratch_asset_importer.rb Implements per-asset importing and optional upload to editor assets bucket via S3 client.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/scratch_asset_importer.rb
Comment thread lib/scratch_asset_importer.rb
Comment on lines +85 to +88
def asset_content_type
extension = File.extname(asset_name).delete('.')
Mime::Type.lookup_by_extension(extension).to_s
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this might apply

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 I might change it to raise or log instead - if a mime type isn't set we should know about it as it means it might not be served correctly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added a new commit that raises if the mime type is unexpected

Comment thread lib/scratch_asset_importer.rb
Comment thread lib/scratch_asset_importer.rb
Copy link
Copy Markdown
Contributor

@abcampo-iry abcampo-iry left a comment

Choose a reason for hiding this comment

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

I think it should be fine specially if import is only used by the task and now implements import_all.

Only that comment about making octet-stream default if returns nil, might be correct since we were doing similar on the editor assets

Comment on lines +85 to +88
def asset_content_type
extension = File.extname(asset_name).delete('.')
Mime::Type.lookup_by_extension(extension).to_s
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this might apply

@zetter-rpf zetter-rpf temporarily deployed to editor-api-p-save-scrat-51jjmk May 26, 2026 14:56 Inactive
As part of this I've registered the 'wav' mimetype as some scratch assets are wav files.
@zetter-rpf zetter-rpf force-pushed the save-scratch-assets-to-bucket branch from a03ef3c to 3f53e45 Compare May 26, 2026 15:15
@zetter-rpf zetter-rpf temporarily deployed to editor-api-p-save-scrat-51jjmk May 26, 2026 15:15 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants