Skip to content
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

Fix issues #10 and #12 #13

Merged
merged 10 commits into from
Dec 4, 2019
5 changes: 1 addition & 4 deletions bin/ci-test-pipeline-1
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ bundle exec rubocop -D --parallel
echo "* ******************************************************"
echo "* Running bundle-audit"
echo "* ******************************************************"
bundle exec bundle-audit update

echo "Running bundle-audit..."
bundle exec bundle-audit check --update --ignore $ignore

echo "* ******************************************************"
Expand All @@ -28,4 +25,4 @@ bundle exec brakeman --exit-on-warn --quiet
echo "* ******************************************************"
echo "* Running rspec unit specs"
echo "* ******************************************************"
bundle exec rspec --format=progress --exclude-pattern "spec/system/**/*_spec.rb"
bundle exec rspec --format=progress --exclude-pattern "spec/system/**/*_spec.rb"
Empty file modified bin/setup
100755 → 100644
Empty file.
5 changes: 2 additions & 3 deletions config/environments/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
RUBY
end


gsub_file "config/environments/development.rb",
"join('tmp/caching-dev.txt')",
'join("tmp", "caching-dev.txt")'
"join('tmp', 'caching-dev.txt')",
'join("tmp/caching-dev.txt")'
42 changes: 36 additions & 6 deletions config/template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
remove_file "config/secrets.yml"
copy_file "config/sidekiq.yml"

gsub_file "config/routes.rb", / # root 'welcome#index'/ do
' root "home#index"'
end

copy_file "config/initializers/generators.rb"
copy_file "config/initializers/rotate_log.rb"
Expand All @@ -24,6 +21,39 @@
apply "config/environments/test.rb"
template "config/environments/staging.rb.tt"

route 'root "home#index"'
route %Q(mount Sidekiq::Web => "/sidekiq" # monitoring console\n)
route "require 'sidekiq/web'"
route <<-EO_ROUTES
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that since this route definition includes three different routes:

  1. Lighthouse workaround
  2. Sidekiq/web
  3. root route

I think we should probably define these with three different route definitions, so that they are inherently three different routes

##
# Workaround a "bug" in lighthouse CLI
#
# Lighthouse CLI (versions 5.4 - 5.6 tested) issues a `GET /asset-manifest.json`
# request during its run - the URL seems to be hard-coded. This file does not
# exist so, during tests, your test will fail because rails will die with a 404.
#
# Lighthouse run from Chrome Dev-tools does not have the same behaviour.
#
# This hack works around this. This behaviour might be fixed by the time you
# read this. You can check by commenting out this block and running the
# accessibility and performance tests. You are encouraged to remove this hack
# as soon as it is no longer needed.
#
if defined?(Webpacker) && Rails.env.test?
# manifest paths depend on your webpacker config so we inspect it
manifest_path = Webpacker::Configuration
.new(root_path: Rails.root, config_path: Rails.root.join("config/webpacker.yml"), env: Rails.env)
.public_manifest_path
.relative_path_from(Rails.root.join("public"))
.to_s
get "/asset-manifest.json", to: redirect(manifest_path)
end

##
# If you want the Sidekiq web console in production environments you need to
# put it behind some authentication first.
#
if defined?(Sidekiq::Web) && Rails.env.development?
mount Sidekiq::Web => "/sidekiq" # Sidekiq monitoring console
require "sidekiq/web"
end

root "home#index"
EO_ROUTES
2 changes: 1 addition & 1 deletion lib/tasks/coverage.rake
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
namespace :test do
task :coverage do
task coverage: :environment do
Copy link
Contributor

Choose a reason for hiding this comment

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

Does loading the environment influence the coverage at all? I was under the impression that simplecov needed to be loaded before the application to corrrectly calculate the coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I did this because rubocop yelled at me about it. I take your point about coverage tho. I'll look into this.

require "simplecov"
Rake::Task["test"].execute
end
Expand Down
2 changes: 1 addition & 1 deletion spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
# directory. Alternatively, in the individual `*_spec.rb` files, manually
# require only the support files necessary.
#
Dir[Rails.root.join("spec", "support", "**", "*.rb")].each { |f| require f }
Dir[Rails.root.join("spec/support/**/*.rb")].each { |f| require f }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rubocop has changed it's mind about separators because / is normalised by Ruby to be whatever the separator on the platform is (e.g. it becomes \ on Windows

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 TIL


# Checks for pending migrations and applies them before tests are run.
# If you are not using ActiveRecord, you can remove these lines.
Expand Down
4 changes: 2 additions & 2 deletions template-test/ci-run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ TEMPLATE=$PWD/template.rb
ROOT=$PWD
gem install rails --no-document

mkdir -p template-test/dummy
mkdir -p template-test/dummy

# Basic run
VARIANT="${VARIANT:-basic}"
rm -rf template-test/dummy/$VARIANT
cd template-test/dummy

echo -e $GENERATOR_INPUT | rails new $VARIANT -d postgresql -m $TEMPLATE
echo -e $GENERATOR_INPUT | RACK_ENV=development RAILS_ENV=development rails new $VARIANT -d postgresql -m $TEMPLATE

cd $ROOT && bash template-test/test.sh template-test/dummy/$VARIANT

Expand Down
51 changes: 27 additions & 24 deletions template.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# encoding: utf-8

require "fileutils"
require "shellwords"

RAILS_REQUIREMENT = "~> 6.0.0".freeze

def apply_template!
Expand Down Expand Up @@ -36,39 +39,39 @@ def apply_template!
apply "public/template.rb"
apply "spec/template.rb"

git :init unless preexisting_git_repo?
empty_directory ".git/safe"
# The block passed to "after_bundle" seems to run after `bundle install`
# but also after `webpacker:install` and after Rails has initialized the git
# repo
after_bundle do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diff is a bit messy but basically I moved lots of commands inside the after_bundle hook. This allowed me to remove our git repo setup and our call to webpacker:install.

run_with_clean_bundler_env "bin/setup"

apply "variants/frontend-base/template.rb"

run_with_clean_bundler_env "bin/setup"
run_with_clean_bundler_env "bin/rails webpacker:install"
create_initial_migration
create_initial_migration

# Apply variants after setup and initial install, but before commit
apply "variants/accessibility/template.rb"
apply "variants/frontend-base/template.rb"
apply "variants/frontend-foundation/template.rb" if apply_variant?(:foundation)
# Apply variants after setup and initial install, but before commit
apply "variants/accessibility/template.rb"
apply "variants/frontend-foundation/template.rb" if apply_variant?(:foundation)

binstubs = %w[
brakeman bundler bundler-audit rubocop sidekiq
]
run_with_clean_bundler_env "bundle binstubs #{binstubs.join(' ')} --force"
binstubs = %w[
brakeman bundler bundler-audit rubocop sidekiq
]
run_with_clean_bundler_env "bundle binstubs #{binstubs.join(' ')} --force"

template "rubocop.yml.tt", ".rubocop.yml"
run_rubocop_autocorrections
template "rubocop.yml.tt", ".rubocop.yml"
run_rubocop_autocorrections

unless any_local_git_commits?
git add: "-A ."
git commit: "-n -m 'Set up project'"
if git_repo_specified?
git remote: "add origin #{git_repo_url.shellescape}"
git push: "-u origin --all"
unless any_local_git_commits?
git add: "-A ."
git commit: "-n -m 'Set up project'"
if git_repo_specified?
git remote: "add origin #{git_repo_url.shellescape}"
git push: "-u origin --all"
end
end
end
end

require "fileutils"
require "shellwords"

# Add this template directory to source_paths so that Thor actions like
# copy_file and template resolve against our source files. If this file was
# invoked remotely via HTTP, that means the files are not present locally.
Expand Down
3 changes: 3 additions & 0 deletions variants/frontend-base/template.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
source_paths.unshift(File.dirname(__FILE__))

run "mv app/assets app/frontend"
run "mkdir app/assets"
run "mv app/frontend/config app/assets/config"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moves app/assets/config/manifest.js back to the place Sprockets requires it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that just Sprockets? I did find putting things in app/frontend to be by far the most brittle part of this template, so I wonder if we should just stick with Rails defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sprockets hard codes this path. rails/sprockets-rails#369 is open to see if it can be made customisable. I take your point on the defaults. I'm loathe to give it up because the default feels gross but accepting the default might be the most pragmatic.


run "mv app/javascript/* app/frontend"
run "rm -rf app/javascript"
apply "config/template.rb"
Expand Down