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

Rails 7.1 updates #36

Merged
merged 15 commits into from
May 31, 2024
Merged

Rails 7.1 updates #36

merged 15 commits into from
May 31, 2024

Conversation

bpurinton
Copy link
Contributor

@bpurinton bpurinton commented May 30, 2024

Resolves #26

Resolves https://github.com/firstdraft/appdev/issues/590

See https://guides.rubyonrails.org/upgrading_ruby_on_rails.html for helpful guide on 7.0 -> 7.1 updating

The steps I took:

  1. Update Gemfile to appropriate rails version (also needed to update puma)
  2. Run bundle update which updates all of the gems in the Gemfile.lock
  3. Run bin/rails app:update, which provides many new files including new DB migration files. Carefully go through all files in the git diff editor in VSCode and modify them to include our special AppDev config settings (e.g. for codespaces)

Testing I did:

  • Open this PR in a codespace
  • Run rails generate devise:install
  • Scaffold a mini blog including User accounts
  • Run rspec
  • Run rails console
  • Play around in the bin/server live preview

Some notes

Devise patch

I was getting some deprecation warnings from Devise that are a known (still unfixed) issue:

heartcombo/devise#5644

I fixed this, per the suggestion on that issue, on the commit: 47ae4f5. This is a bit awkward but I don't see another work around until they make an official fix to the gem.

New DB migration files

The bin/rails app:update command generated three new migration files. I'm not sure if / how these can be removed.

@@ -1,4 +1,3 @@
--color
--format documentation
--order default
Copy link
Contributor Author

@bpurinton bpurinton May 30, 2024

Choose a reason for hiding this comment

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

This option seemed to have become deprecated and caused rspec to fail (hard to find info about where this happened after awhile searching online!). In any case, the default option is --order defined (see docs), so we don't need to specify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this happened in Rspec v3, which is quite old. I'm surprised we didn't notice warnings or issues from the beginning 😅

@bpurinton bpurinton marked this pull request as ready for review May 30, 2024 18:38
Copy link
Contributor

@jelaniwoods jelaniwoods left a comment

Choose a reason for hiding this comment

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

You can remove the migration files, They aren't applicable to this project. I also suggest dropping the database so the schema is blank when students start using the template.

@@ -1,4 +1,3 @@
--color
--format documentation
--order default
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this happened in Rspec v3, which is quite old. I'm surprised we didn't notice warnings or issues from the beginning 😅

# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
# config.force_ssl = true
config.force_ssl = true
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 we still want to comment this out in case students don't have a plan that allows SSL when they deploy.

IIRC you don't get SSL for free Heroku apps, but maybe Render and Fly.io already allow this, I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I missed that change.

@@ -0,0 +1,27 @@
# This migration comes from active_storage (originally 20191206030411)
class CreateActiveStorageVariantRecords < ActiveRecord::Migration[6.0]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this migration too for similar reasons. Since this is a template project and there is no database, (and active storage isn't being used) we don't need to run this migration since there's no existing data that we need to migrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying about these migration files. I've removed them, reset the schema to version 0, and re-ran bin/setup in a codespace to reset the development.sqlite3 and test.sqlite3 DBs and added those empty database files to this PR. These DBs were also already per-generated on main on a commit from last year: 1f54f85

@bpurinton bpurinton requested a review from jelaniwoods May 30, 2024 23:00
@bpurinton bpurinton mentioned this pull request May 30, 2024
@bpurinton
Copy link
Contributor Author

I'm going to merge this as is, as it's blocking me on some things. I took care of the fairly straightforward review comments already.

@bpurinton bpurinton merged commit cfcd120 into main May 31, 2024
@bpurinton bpurinton deleted the bp-rails-7-1-updates branch May 31, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade rails to ~> 7.1
2 participants