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

Resolve Rails 8 upgrade issues #614

Merged
merged 3 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion bin/dev
Original file line number Diff line number Diff line change
@@ -1,2 +1,30 @@
#!/usr/bin/env ruby
exec "./bin/rails", "server", *ARGV
# frozen_string_literal: true

def installed?(process)
IO.popen "#{process} -v"
rescue Errno::ENOENT
false
end
Comment on lines +4 to +8
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance security of process version check.

The current implementation using IO.popen could be vulnerable to command injection. Consider using which command instead.

 def installed?(process)
-  IO.popen "#{process} -v"
+  system("which #{process.shellescape} > /dev/null 2>&1")
-rescue Errno::ENOENT
-  false
 end

Committable suggestion skipped: line range outside the PR's diff.


def run(process)
system "#{process} start -f Procfile.dev"
rescue Errno::ENOENT
warn <<~MSG
ERROR:
Please ensure `Procfile.dev` exists in your project!
MSG
exit!
end

if installed? "overmind"
run "overmind"
elsif installed? "foreman"
run "foreman"
else
warn <<~MSG
NOTICE:
For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them.
MSG
exit!
end
4 changes: 4 additions & 0 deletions bin/setup
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ FileUtils.chdir APP_ROOT do
# Add necessary setup steps to this file.

puts "== Installing dependencies =="
system! "gem install bundler --conservative"
system("bundle check") || system!("bundle install")

# Install JavaScript dependencies
system! "bin/yarn"

Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add Node.js and Yarn version verification.

Verify Node.js and Yarn versions meet the minimum requirements for Rails 8.

   # Install JavaScript dependencies
+  required_node_version = "18.0.0"
+  required_yarn_version = "1.22.0"
+  
+  puts "\n== Verifying Node.js and Yarn versions =="
+  system! "node -v | grep -q 'v#{required_node_version}' || (echo 'Node.js #{required_node_version}+ is required'; exit 1)"
+  system! "yarn -v | grep -q '#{required_yarn_version}' || (echo 'Yarn #{required_yarn_version}+ is required'; exit 1)"
+
   system! "bin/yarn"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Install JavaScript dependencies
system! "bin/yarn"
# Install JavaScript dependencies
required_node_version = "18.0.0"
required_yarn_version = "1.22.0"
puts "\n== Verifying Node.js and Yarn versions =="
system! "node -v | grep -q 'v#{required_node_version}' || (echo 'Node.js #{required_node_version}+ is required'; exit 1)"
system! "yarn -v | grep -q '#{required_yarn_version}' || (echo 'Yarn #{required_yarn_version}+ is required'; exit 1)"
system! "bin/yarn"

# puts "\n== Copying sample files =="
# unless File.exist?("config/database.yml")
# FileUtils.cp "config/database.yml.sample", "config/database.yml"
Expand Down
2 changes: 1 addition & 1 deletion config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class Application < Rails::Application
# These settings can be overridden in specific environments using the files
# in config/environments, which are processed later.

config.active_support.to_time_preserves_timezone = :zone
config.active_support.to_time_preserves_timezone = false
# config.time_zone = "Central Time (US & Canada)"
# config.eager_load_paths << Rails.root.join("extras")
end
Expand Down
1 change: 1 addition & 0 deletions config/database.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ default: &default

development:
<<: *default
database: react-webpack-rails-tutorial
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using environment-specific database names.

Using the same database name across all environments could lead to accidental data manipulation if environment variables are misconfigured.

 development:
   <<: *default
-  database: react-webpack-rails-tutorial
+  database: react-webpack-rails-tutorial_development

 test:
   <<: *default
-  database: react-webpack-rails-tutorial
+  database: react-webpack-rails-tutorial_test

 production:
   <<: *default
-  database: react-webpack-rails-tutorial
+  database: <%= ENV.fetch("POSTGRES_DATABASE", "react-webpack-rails-tutorial_production") %>

Also applies to: 42-42, 47-47

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing PostgreSQL configurations.

The PostgreSQL configuration is missing essential settings that could affect performance and connectivity.

 default: &default
   adapter: postgresql
+  encoding: unicode
+  pool: <%= ENV.fetch("RAILS_MAX_THREADS") { 5 } %>
+  username: <%= ENV.fetch("POSTGRES_USER", "postgres") %>
+  password: <%= ENV.fetch("POSTGRES_PASSWORD") { "" } %>
+  host: <%= ENV.fetch("POSTGRES_HOST", "localhost") %>
+  port: <%= ENV.fetch("POSTGRES_PORT", 5432) %>

Committable suggestion skipped: line range outside the PR's diff.


# Warning: The database defined as "test" will be erased and
# re-generated from your development database when you run "rake".
Expand Down
18 changes: 12 additions & 6 deletions config/environments/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@
if Rails.root.join("tmp/caching-dev.txt").exist?
config.action_controller.perform_caching = true
config.action_controller.enable_fragment_cache_logging = true
config.cache_store = :memory_store
config.public_file_server.headers = { "cache-control" => "public, max-age=#{2.days.to_i}" }
else
config.cache_store = :null_store
config.action_controller.perform_caching = false
end

# Change to :null_store to avoid any caching.
config.cache_store = :memory_store

# Store uploaded files on the local file system (see config/storage.yml for options).
config.active_storage.service = :local

Expand All @@ -39,12 +38,15 @@
# Make template changes take effect immediately.
config.action_mailer.perform_caching = false

# Set localhost to be used by links generated in mailer templates.
config.action_mailer.default_url_options = { host: "localhost", port: 3000 }

# Print deprecation notices to the Rails logger.
config.active_support.deprecation = :log

# Raise exceptions for disallowed deprecations.
config.active_support.disallowed_deprecation = :raise

# Tell Active Support which deprecation messages to disallow.
config.active_support.disallowed_deprecation_warnings = []

# Raise an error on page load if there are pending migrations.
config.active_record.migration_error = :page_load

Expand All @@ -63,6 +65,10 @@
# Annotate rendered view with file names.
config.action_view.annotate_rendered_view_with_filenames = true

# Use an evented file watcher to asynchronously detect changes in source code,
# routes, locales, etc. This feature depends on the listen gem.
config.file_watcher = ActiveSupport::EventedFileUpdateChecker

# Uncomment if you wish to allow Action Cable access from any origin.
# config.action_cable.disable_request_forgery_protection = true

Expand Down
55 changes: 48 additions & 7 deletions config/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@
# Turn on fragment caching in view templates.
config.action_controller.perform_caching = true

# Ensures that a master key has been made available in either ENV["RAILS_MASTER_KEY"]
# or in config/master.key. This key is used to decrypt credentials (and other encrypted files).
# config.require_master_key = true

# Disable serving static files from the `/public` folder by default since
# Apache or NGINX already handles this.
config.public_file_server.enabled = ENV["RAILS_SERVE_STATIC_FILES"].present?

# Cache assets for far-future expiry since they are all digest stamped.
config.public_file_server.headers = { "cache-control" => "public, max-age=#{1.year.to_i}" }

Expand All @@ -27,10 +35,10 @@
config.active_storage.service = :local

# Assume all access to the app is happening through a SSL-terminating reverse proxy.
config.assume_ssl = true
# config.assume_ssl = true
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security Concern: SSL configuration is commented out.

In production, it's crucial to enforce SSL for security. Consider enabling these configurations:

-  # config.assume_ssl = true
+  config.assume_ssl = true

-  # config.force_ssl = true
+  config.force_ssl = true

Also applies to: 41-41


# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
config.force_ssl = true
# config.force_ssl = true

# Skip http-to-https redirect for the default health check endpoint.
# config.ssl_options = { redirect: { exclude: ->(request) { request.path == "/up" } } }
Expand All @@ -45,9 +53,6 @@
# Prevent health checks from clogging up the logs.
config.silence_healthcheck_path = "/up"

# Don't log any deprecations.
config.active_support.report_deprecations = false

# Replace the default in-process memory cache store with a durable alternative.
# config.cache_store = :mem_cache_store

Expand All @@ -59,7 +64,7 @@
# config.action_mailer.raise_delivery_errors = false

# Set host to be used by links generated in mailer templates.
config.action_mailer.default_url_options = { host: "example.com" }
# config.action_mailer.default_url_options = { host: "example.com" }

# Specify outgoing SMTP server. Remember to add smtp/* credentials via rails credentials:edit.
# config.action_mailer.smtp_settings = {
Expand All @@ -74,11 +79,36 @@
# the I18n.default_locale when a translation cannot be found).
config.i18n.fallbacks = true

# Don't log any deprecations.
# config.active_support.report_deprecations = false

# Send deprecation notices to registered listeners.
config.active_support.deprecation = :notify

# Log disallowed deprecations.
config.active_support.disallowed_deprecation = :log

# Tell Active Support which deprecation messages to disallow.
config.active_support.disallowed_deprecation_warnings = []

# Use default logging formatter so that PID and timestamp are not suppressed.
config.log_formatter = ::Logger::Formatter.new

# Use a different logger for distributed setups.
# require "syslog/logger"
# config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

if ENV["RAILS_LOG_TO_STDOUT"].present?
logger = ActiveSupport::Logger.new($stdout)
logger.formatter = config.log_formatter
config.logger = ActiveSupport::TaggedLogging.new(logger)
end

# Do not dump schema after migrations.
config.active_record.dump_schema_after_migration = false

# Only use :id for inspections in production.
config.active_record.attributes_for_inspect = [:id]
# config.active_record.attributes_for_inspect = [:id]

# Enable DNS rebinding protection and other `Host` header attacks.
# config.hosts = [
Expand All @@ -88,4 +118,15 @@
#
# Skip DNS rebinding protection for the default health check endpoint.
# config.host_authorization = { exclude: ->(request) { request.path == "/up" } }

# strategy for connection switching and pass that into the middleware through
# these configuration options.
# config.active_record.database_selector = { delay: 2.seconds }
# config.active_record.database_resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver
# config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session

# Action Cable endpoint configuration

config.action_cable.url = "wss://#{ENV['PRODUCTION_HOST']}/cable"
config.action_cable.allowed_request_origins = ["https://#{ENV['PRODUCTION_HOST']}"]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use ENV.fetch for safer environment variable handling.

Using ENV[] can lead to nil values if variables are missing. Use ENV.fetch with a default value or error message.

-  config.action_cable.url = "wss://#{ENV['PRODUCTION_HOST']}/cable"
-  config.action_cable.allowed_request_origins = ["https://#{ENV['PRODUCTION_HOST']}"]
+  config.action_cable.url = "wss://#{ENV.fetch('PRODUCTION_HOST', 'localhost')}/cable"
+  config.action_cable.allowed_request_origins = ["https://#{ENV.fetch('PRODUCTION_HOST', 'localhost')}"]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
config.action_cable.url = "wss://#{ENV['PRODUCTION_HOST']}/cable"
config.action_cable.allowed_request_origins = ["https://#{ENV['PRODUCTION_HOST']}"]
config.action_cable.url = "wss://#{ENV.fetch('PRODUCTION_HOST', 'localhost')}/cable"
config.action_cable.allowed_request_origins = ["https://#{ENV.fetch('PRODUCTION_HOST', 'localhost')}"]
🧰 Tools
🪛 GitHub Actions: Lint CI

[warning] 130-130: Use ENV.fetch('PRODUCTION_HOST') or ENV.fetch('PRODUCTION_HOST', nil) instead of ENV['PRODUCTION_HOST']


[warning] 131-131: Use ENV.fetch('PRODUCTION_HOST') or ENV.fetch('PRODUCTION_HOST', nil) instead of ENV['PRODUCTION_HOST']

end
2 changes: 1 addition & 1 deletion config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
config.action_mailer.delivery_method = :test

# Set host to be used by links generated in mailer templates.
config.action_mailer.default_url_options = { host: "example.com" }
# config.action_mailer.default_url_options = { host: "example.com" }

# Print deprecation notices to the stderr.
config.active_support.deprecation = :stderr
Expand Down
64 changes: 0 additions & 64 deletions config/initializers/new_framework_defaults_6_1.rb

This file was deleted.

Loading
Loading