New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Active Storage to Rails #30020

Merged
merged 396 commits into from Aug 4, 2017

Conversation

Projects
None yet
@rafaelfranca
Member

rafaelfranca commented Jul 31, 2017

Need to address a few issues:

  • How should the initial migration be seeded? I'm thinking probably just generate it in db/migrations on new apps.
  • Move the direct_upload: true convenience option from the activestorage helper into actionview.
  • Address how Rails' CI can run the cloud service integration tests.

jeremy and others added some commits Jul 8, 2017

Disk storage: ensure URLs end with the blob filename since some user …
…agents don't respect Content-Disposition filename
Merge pull request #22 from jeremy/disk-filename-url
Disk storage: ensure URLs end with the blob filename
Merge pull request #21 from jeremy/disk-binary-io
Disk service: use binary IO throughout, not UTF-8
Mirror: explicit primary service and list of mirrors
Pass separate primary service and list of mirrors rather than treating
the first of the services list as the primary. Nice fit for keyword args,
and something we've long wanted in the equivalent Basecamp file repository.

Upload returns the results of the underlying service uploads rather than
the io.rewind result. Rewind before uploading rather than afterward, and
demonstrate that behavior with a test.

Test that more than one mirror works.
Configure services that reference other services
* Move service configuration from the Engine to Service
* Delegate configuration mechanics to internal Service::Configurator
* Delegate service building to the concrete Service classes, allowing
  them to configure composed services.
* Implement for the Mirror service.
Service: clarify Service.build arguments
First arg is config for the service we're instantiating.

Second arg is service configurations so we can look up and configure
other services by name.
Clarify how a service can build other composed services
* Service.build takes the literal YAML config hash for the service and a
  reference to the Configurator that's doing the building.
* Services that compose additional services can use the Configurator to
  look them up and build them by name. See MirrorService for an example.

References #23
Merge pull request #26 from jeremy/service-configurator
Clarify how a service can build other composed services
Symbolize all keys inside configuration nested hash
Since configuration is a nested hash we need to symbolize all keys
of the hash. Othervise fetcing will fail on start
Merge pull request #27 from dixpac/fix_configuration_keys_symbolization
Symbolize all keys inside configuration nested hash
Copypasta comments
# Conflicts:
#	lib/active_storage/engine.rb
#	lib/active_storage/service.rb
#	lib/active_storage/service/disk_service.rb
#	lib/active_storage/service/s3_service.rb
#	test/service/s3_service_test.rb
#	test/test_helper.rb
Configurator tests: work against test-local config
So tests pass when service configs aren't set up.

References #28
Scope aws-skd to version 2 (#34)
Since we use new aws-sdk API, I've scoped aws-sdk version
so someone doesn't accidentaly installs wrong version during the
development.
S3: slim down service implementation (#40)
* Use simple core API for duck-type compat with other clients
* initialize: accept an existing client
* initialize: accept arbitrary client args instead of a fixed, required set
* download: use native get_object streaming, no need to implement range requests
* exists?: use head_object (which returns immediately) rather than waiting for existence
@@ -0,0 +1,22 @@
# Take a signed permanent reference for a blob and turn it into an expiring service URL for download.

This comment has been minimized.

@claudiob

claudiob Aug 3, 2017

Member

How are these four controllers in app/controllers/active_storage intended to be used in a Rails app?
Are they meant as superclasses to inherit from?

@claudiob

claudiob Aug 3, 2017

Member

How are these four controllers in app/controllers/active_storage intended to be used in a Rails app?
Are they meant as superclasses to inherit from?

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 3, 2017

Member

Are they meant as superclasses to inherit from?

No, though you could probably extend them if you so desired. Apps use them by sending requests to them, either via routes or DiskService#url{,_for_direct_upload}.

@georgeclaghorn

georgeclaghorn Aug 3, 2017

Member

Are they meant as superclasses to inherit from?

No, though you could probably extend them if you so desired. Apps use them by sending requests to them, either via routes or DiskService#url{,_for_direct_upload}.

This comment has been minimized.

@kaspth

kaspth Aug 3, 2017

Member

Active Storage is a Rails::Engine so the controllers (and their routes) are available in users' Rails apps 😄

@kaspth

kaspth Aug 3, 2017

Member

Active Storage is a Rails::Engine so the controllers (and their routes) are available in users' Rails apps 😄

dhh and others added some commits Aug 3, 2017

Make sure Action View doesn't break with Active Storage
When Active Storage is not loaded and direct_upload is used on
file_field_tag we should not raise an exception.
Refactor convert_direct_upload_option_to_url
Also make sure file_field doesn't mutate the original options passed in.
Remove duplicated convert_direct_upload_option_to_url
FormHelper includes FormTagHelper so we don't need to define two methods
Active Storage routes are now part of the default routes
It's worth considering whether we should hide these by default, but I'm kinda thinking no. It's very reasonable that someone would want to call these directly, so they should be documented.
Do not eager load ActiveRecord::Base
Everything inside the app directory of a engine is autoload/eager loaded automatically so we don't need to require them.
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 3, 2017

Member

❤️

Member

dhh commented on f601a01 Aug 3, 2017

❤️

rafaelfranca and others added some commits Aug 3, 2017

Merge pull request #30061 from claudiob/ast-rubocop
Make Rubocop happy about ActiveStorage
Do not change the entire codebase style only because of active_storage
We are already removing the braces around hash parameters in the last
argument in other places so we should not change the entire codebase
because of two places.
Include active_storage in all generated application.rb files
And deal with a temporary test fix until we allow you to skip active storage.
Do not call isolate_namespace on the application
What we want to test is that two different calls to isolate_namespace
with the same module doesn't change the original railtie. We can do that
defining two different railties.

We can't call in the application because this method is not supposed to
be called in an Application class.
Make sure the test fails if the code chages
The test was passing before because it was not being testes correctly.

Now we create a different engine that is loaded before the already
exising and we make sure that the first call for isolate_namespace is
what takes effect.
Fix tests for AWS buckets that include a . (#30059)
If an AWS bucket name includes a `.` (e.g. `bucket.name`), then the canonical
URL for an object will start with "https://s3.amazonaws.com/bucket.name/"
and not with "https://bucket.name.s3.amazonaws.com/".

The URL tests have now been separated into two separate asserts, to ensure
that both the "s3.amazonaws.com" and the "bucket.name" components are included,
but not specifically in that order.

@dhh dhh merged commit 5528406 into master Aug 4, 2017

0 of 3 checks passed

codeclimate 3 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 5, 2017

Member

Wups! It works! ❤️

Member

kaspth commented Aug 5, 2017

Wups! It works! ❤️

@georgeclaghorn georgeclaghorn deleted the active-storage-import branch Aug 14, 2017

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Feb 11, 2018

Member

@georgeclaghorn Currently

you must add gem "mini_magick" to your Gemfile if you wish to use variants.

This makes sense but I was wondering… what if your code didn't raise an error if you don't have mini_magick and just threw a warning in the log? In other words, avatar.variant(resize: "100x100") would simply return the original image with a warning.

It's just a thought, inspired by the fact that ImageAnalyzer does exactly that:

    rescue LoadError
      logger.info "Skipping image analysis because the mini_magick gem isn't installed"
Member

claudiob commented on app/models/active_storage/variant.rb in ac26aef Feb 11, 2018

@georgeclaghorn Currently

you must add gem "mini_magick" to your Gemfile if you wish to use variants.

This makes sense but I was wondering… what if your code didn't raise an error if you don't have mini_magick and just threw a warning in the log? In other words, avatar.variant(resize: "100x100") would simply return the original image with a warning.

It's just a thought, inspired by the fact that ImageAnalyzer does exactly that:

    rescue LoadError
      logger.info "Skipping image analysis because the mini_magick gem isn't installed"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment