Skip to content

Feedback PR for #2287: Story 2269: Enable Conditional S3 Storage for development#2291

Merged
jlchilders11 merged 10 commits intojc/conditional-image-storagefrom
teo/jc/image-sync-feedback
Apr 14, 2026
Merged

Feedback PR for #2287: Story 2269: Enable Conditional S3 Storage for development#2291
jlchilders11 merged 10 commits intojc/conditional-image-storagefrom
teo/jc/image-sync-feedback

Conversation

@herzog0
Copy link
Copy Markdown
Collaborator

@herzog0 herzog0 commented Apr 13, 2026

⚠️ Base branch is jc/conditional-image-storage

Feedback for PR #2287

Issue: 2269

Changes

  • Renamed the original upload-images.sh script to sync-large-static-images.sh;
  • Encapsulated the behavior of the uploads images into a function in the sync script;
  • Changed the behavior of the upload function to "up sync", using the same strategy to down sync, i.e., it won't copy all files from SOURCE_DIR to aws every time, it'll just sync the static-large/ folder upwards too, which is totally safe as it won't delete anything in the remote buckets;
    • Therefore I removed the section that offers to delete the source dir after upload;
  • Added the "down-sync" capabilities to the script;
  • Defaults to uploading to the staging bucket rather than all of them;
  • Changed/added the commands in justfile:
    • down_sync_images; → now calls the new script
    • up_sync_images; → defaults to uploading to staging bucket only;
    • up_sync_images_all_buckets: → uploads to all buckets
  • Changed the AWS profile from upload-images to sync-boost-images for better comprehension (we may have credentials for multiple projects in our local machines);
  • Added documentation in the readme file;

@herzog0 herzog0 changed the title Teo/jc/image sync feedback Feedback PR for #2287: Story 2269: Enable Conditional S3 Storage for development Apr 13, 2026
Copy link
Copy Markdown
Collaborator

@jlchilders11 jlchilders11 left a comment

Choose a reason for hiding this comment

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

Some thoughts on the implementation, but I do like the basic concept, pending Sam's approval.

Comment thread scripts/sync-large-static-images.sh Outdated
Comment thread scripts/sync-large-static-images.sh Outdated
Comment thread scripts/sync-large-static-images.sh Outdated
@jlchilders11 jlchilders11 linked an issue Apr 13, 2026 that may be closed by this pull request
Comment thread scripts/sync-large-static-images.sh Outdated
@herzog0 herzog0 force-pushed the teo/jc/image-sync-feedback branch from 0df3dd6 to 689ca19 Compare April 14, 2026 15:22
Copy link
Copy Markdown
Collaborator

@jlchilders11 jlchilders11 left a comment

Choose a reason for hiding this comment

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

LGTM - I like this approach

Copy link
Copy Markdown
Collaborator

@julhoang julhoang left a comment

Choose a reason for hiding this comment

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

LGTM! Just 1 tiny nit for an outdated note

Comment thread scripts/sync-large-static-images.sh Outdated
@jlchilders11 jlchilders11 merged commit 4cf4ef4 into jc/conditional-image-storage Apr 14, 2026
4 checks passed
@jlchilders11 jlchilders11 deleted the teo/jc/image-sync-feedback branch April 14, 2026 17:46
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.

Enable Conditional S3 Storage for large static content

4 participants