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

Provide initialization of Active Storage #30101

Merged
merged 4 commits into from Nov 6, 2017

Conversation

Projects
None yet
8 participants
@bogdanvlviv
Contributor

bogdanvlviv commented Aug 6, 2017

#30098 (comment)

Closes #30102

@rails-bot

This comment has been minimized.

rails-bot commented Aug 6, 2017

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@bogdanvlviv bogdanvlviv changed the title from WIP: Provide initialization Active Storage when `rails new` to WIP: Provide initialization Active Storage Aug 7, 2017

@bogdanvlviv bogdanvlviv changed the title from WIP: Provide initialization Active Storage to Provide initialization of Active Storage Aug 7, 2017

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 7, 2017

After adding Active Storage by default, new apps take up more space.
I got the error Errno::ENOSPC: No space left on device - sendfile on travis-ci(https://travis-ci.org/rails/rails/builds/261890209) and my virtual machine.
The reason that we don't delete a generated app after finishing a test case, I looked at size /tmp and saw that it took up more than 4G space after rails/railties$ bundle exec rake test.
Looks like we must remove a generated app after a test's finishing.

This commit fixes it 02b64f4, but now there is another problem, it takes up more time for execution(https://travis-ci.org/rails/rails/builds/261934074).
Does anyone have any idea?

railties/lib/rails/generators/app_base.rb Outdated
end
def comment_if(value) # :doc:
options[value] ? "# " : ""
comment =
if value.is_a?(String) || value.is_a?(Symbol)

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 8, 2017

Member

instead of type check better to check if the class respond to the value

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 9, 2017

Contributor

I Agree. Thanks.

railties/lib/rails/generators/rails/plugin/templates/rails/application.rb Outdated
require "action_controller/railtie"
require "action_view/railtie"
<%= comment_if :skip_action_mailer %>require "action_mailer/railtie"
require "active_job/railtie"
require "active_storage/engine"
<%= comment_if :skip_action_cable %>require "action_cable/engine"

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 8, 2017

Member

Why this was removed?

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 9, 2017

Contributor

Oops. I missed.

@rafaelfranca rafaelfranca added this to the 5.2.0 milestone Aug 8, 2017

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Aug 11, 2017

Looking good so far. Could you rebase and fix the tests?

@willnet

This comment has been minimized.

Contributor

willnet commented Aug 12, 2017

requiring activestorage.js looks to need to be removed on use--skip-active-storage

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 13, 2017

@willnet Thanks. Now it is covered by this PR.

@@ -326,6 +326,10 @@ def create_system_test_files
build(:system_test) if depends_on_system_test?
end
def create_storage_files
build(:storage) unless skip_active_storage?

This comment has been minimized.

@yhirano55

yhirano55 Aug 13, 2017

Contributor

It seems that it does not need to consider skip flag here. If it enabled skip flag for active storage, all files will be removed on #delete_storage_files_skipping_active_storege.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 13, 2017

Contributor

thanks i just removed #delete_storage_files_skipping_active_storege as it isn't needed

@rafaelfranca

Can you squash the commit that add the CHANGELOG in the commit that add that feature?

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 15, 2017

@rafaelfranca Thank you for the review!
Currently, i have to fix failed tests in railties/test/railties/engine_test.rb
Also, i think we can merge it only after fixing the The job exceeded the maximum time limit for jobs, and has been terminated. to prevent failing next builds on CI.
I've mentioned it here #30101 (comment) and in Campfire.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Aug 15, 2017

Maybe something in your PR is adding more time to the build since I don't see it happening on master. Could you check which change is doing that?

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 17, 2017

@rafaelfranca
It definitely my changes add more time to build tests, since rails new runs rails active_storage:install by default.

I've got two problems since adding that:
The first related to memory, it is described in the commit of this PR Fix error Errno::ENOSPC: No space left on device - sendfile,
The second related to time, Rails::Generators::Testing::Behaviour#run_generator skips Active Storage by default, but I'm not sure that is the right.

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 20, 2017

@rafaelfranca

#30101 (comment)
#30101 (comment)

This PR is blocked by long-time execution of the tests.
I opened new PR(#30304) that can help resolve the long-time test execution problem.

I've tested it in 2 ways:

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Aug 24, 2017

I will continue to work on this when I get more info about nixing rails active_storage:install. See
#30280 (comment)

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Sep 14, 2017

@rafaelfranca This PR is ready for review.

Just want to notice one more time that railties' tests became slower with execution of rails active_storage:install for new apps by default.

Recently CI was falling for this PR because of Travis CI' time limit. After merging this #30520(Thanks @matthewd), tests got time to be executed under this PR.

I'm not sure should we wait for replacing of active_storage:install rake task, see #30280 (comment)
/cc @dhh

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 10, 2017

The test is failed because
execution of rails active_storage:install can't respect option --quiet.
(Related to #30700 /cc @y-yagi )

What do you think about copying ActiveStorage' migration to railties/lib/rails/generators/rails/app/templates/db/migrate/?

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 10, 2017

@georgeclaghorn Do you have any replacement for rails active_storage:install? #30378 (comment)

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Oct 10, 2017

@sgrif was looking into an alternative to the installation task, if I remember correctly.

@y-yagi

This comment has been minimized.

Member

y-yagi commented Oct 11, 2017

@bogdanvlviv
I think it's a good to add capture option support to execute_command. As described below.

diff --git a/railties/lib/rails/generators/actions.rb b/railties/lib/rails/generators/actions.rb
index 9800e57..ea85c49 100644
--- a/railties/lib/rails/generators/actions.rb
+++ b/railties/lib/rails/generators/actions.rb
@@ -292,7 +292,8 @@ def execute_command(executor, command, options = {}) # :doc:
           log executor, command
           env  = options[:env] || ENV["RAILS_ENV"] || "development"
           sudo = options[:sudo] && !Gem.win_platform? ? "sudo " : ""
-          in_root { run("#{sudo}#{extify(executor)} #{command} RAILS_ENV=#{env}", verbose: false) }
+          capture = options[:capture] || false
+          in_root { run("#{sudo}#{extify(executor)} #{command} RAILS_ENV=#{env}", verbose: false, capture: capture) }
         end
 
         # Add an extension to the given name based on the platform.
diff --git a/railties/lib/rails/generators/app_base.rb b/railties/lib/rails/generators/app_base.rb
index b33fd35..778fb83 100644
--- a/railties/lib/rails/generators/app_base.rb
+++ b/railties/lib/rails/generators/app_base.rb
@@ -461,7 +461,7 @@ def generate_spring_binstubs
 
       def run_active_storage
         unless skip_active_storage?
-          rails_command "active_storage:install"
+          rails_command "active_storage:install", capture: options[:quiet]
         end
       end
:skip_sprockets,
:skip_action_cable
),
skip_active_storage?

This comment has been minimized.

@y-yagi

y-yagi Oct 11, 2017

Member

I think that specifying skip_active_storage directly is more simpler.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Oct 11, 2017

Contributor

Yeah it is simpler, but I think it is more correctly use skip_active_storage?

@y-yagi

This comment has been minimized.

Member

y-yagi commented Oct 11, 2017

We also need skip_active_storage? check in template of Gemfile.

@@ -153,6 +153,7 @@ def build_app(options = {})
def teardown_app
ENV["RAILS_ENV"] = @prev_rails_env if @prev_rails_env
FileUtils.rm_rf(tmp_path)

This comment has been minimized.

@y-yagi

y-yagi Oct 11, 2017

Member

Why was this added?

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Oct 11, 2017

Contributor

Intentionally, see description of commit Fix error Errno::ENOSPC: No space left on device - sendfile

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 11, 2017

The tests are green!

Thanks @y-yagi

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 6, 2017

Could you squash the commits?

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Nov 6, 2017

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 6, 2017

That are still too many commits related to the same change that can be squashed.

Add --skip-active-storage and do so automatically when --skip-active-…
…record is used

Closes #30102

Revert part 787fe90

--skip-active-storage pass throughs `rails plugin new`

Add changelog entry about default initialization of Active Storage
@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Nov 6, 2017

I squashed one more time.

bogdanvlviv added some commits Aug 7, 2017

`rails new` runs `rails active_storage:install`
Omit `rails activestorage:install` for jdbcmysql, jdbc and shebang tests

AppGeneratorTest#test_config_jdbcmysql_database

  rails aborted!
  LoadError: Could not load 'active_record/connection_adapters/mysql_adapter'.
  Make sure that the adapter in config/database.yml is valid.
  If you use an adapter other than 'mysql2', 'postgresql' or 'sqlite3' add
  the necessary adapter gem to the Gemfile.
  (compressed)
  bin/rails:4:in `<main>'
  Tasks: TOP => activestorage:install => environment
  (See full trace by running task with --trace)

AppGeneratorTest#test_config_jdbc_database

  rails aborted!
  LoadError: Could not load 'active_record/connection_adapters/jdbc_adapter'.
  Make sure that the adapter in config/database.yml is valid.
  If you use an adapter other than 'mysql2', 'postgresql' or 'sqlite3' add
  the necessary adapter gem to the Gemfile.
  (compressed)
  bin/rails:4:in `<main>'
  Tasks: TOP => activestorage:install => environment
  (See full trace by running task with --trace)

AppGeneratorTest#test_shebang_is_added_to_rails_file

  /home/ubuntu/.rbenv/versions/2.4.1/bin/ruby: no Ruby script found in input (LoadError)

Prevent PendingMigrationError in tests

 * Run `bin/rails db:migrate RAILS_ENV=test` in test_cases before start tests to prevent PendingMigrationError
 * FileUtils.rm_r("db/migrate")
 * --skip-active-storage

Fix failed tests in `railties/test/railties/engine_test.rb`

Related to #30111

Imporve `SharedGeneratorTests#test_default_frameworks_are_required_when_others_are_removed`

 - Explicitly skip active_storage
 - Ensure that skipped frameworks are commented
 - Ensure that default frameworks are not commented

Fix error `Errno::ENOSPC: No space left on device - sendfile`

Since `rails new` runs `rails active_storage:install`
that boots an app.

Since adding Bootsnap 0312a5c
during booting an app, it creates the cache:

   264K    tmp/cache/bootsnap-load-path-cache
   27M     tmp/cache/bootsnap-compile-cache

* teardown_app must remove app

@rafaelfranca rafaelfranca merged commit 63f0c04 into rails:master Nov 6, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:initialization-active_storage branch Nov 6, 2017

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Nov 8, 2017

Remove redundant passing --skip-active-storage in test cases
These were added in rails#30101, after rails#31084 it became redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment