Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Add local file storage for dev environments #2928

Merged
merged 4 commits into from
Apr 23, 2018

Conversation

johnnynotsolucky
Copy link
Contributor

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • api

Release notes for users (delete if codebase-only change)

  • This adds an option to allow local storage of uploaded images instead of storing to S3.
  • The change is purely opt-in, so without using the FILE_STORAGE env variable, it'll default to original S3 logic.
  • Local storage points to the public/uploads dir, which will be created if it doesn't exist
  • public/uploads has been added to .gitignore

This PR will allow contributors to work with image uploads without requiring AWS keys/secrets. It should also be easier to add new implementations in the future.

If this is something that y'all would like to have, it would be good to test with and without AWS keys.

@brianlovin
Copy link
Contributor

This is awesome! One request: could we add documentation around this to the setup guide? Might even call for a new subsection "Working with image uploads locally" or something

Will test this locally after that!

@johnnynotsolucky
Copy link
Contributor Author

@brianlovin I think its good to go.

Flow errors have been fixed, and I've added some notes to the README under starting the API.

@brianlovin
Copy link
Contributor

Sorry, now that I'm actually using this - what would you think of just setting that FILE_STORAGE=local flag by default in the dev:api script? Then we can remove extra information overhead for new contributors.

@johnnynotsolucky
Copy link
Contributor Author

Sorted. I was worried that it would affect the dev experience for people already working on Spectrum, but upload returns URLs, so images that have already been uploaded to S3 should still show up fine.

@brianlovin brianlovin requested a review from mxstbr April 22, 2018 21:50
@brianlovin
Copy link
Contributor

This looks great to me and works locally - would love a look from @mxstbr but otherwise this is good to go :)

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you so much!

@mxstbr mxstbr merged commit e220fef into withspectrum:alpha Apr 23, 2018
@johnnynotsolucky johnnynotsolucky deleted the local-image-storage branch April 23, 2018 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants