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

Add Rails 7.1 support #6

Merged
merged 17 commits into from
Mar 22, 2024
Merged

Add Rails 7.1 support #6

merged 17 commits into from
Mar 22, 2024

Conversation

argvniyx-enroute
Copy link
Collaborator

@argvniyx-enroute argvniyx-enroute commented Mar 20, 2024

/task https://app.asana.com/0/1205835826533561/1205835897803467/f

This adds Rails 7.1 support and then some. I'll try to explain inline. Empty lines were added by running rubocop after adding the 7.1 gemfile.

Qs:

  • Why do we have two establish_connection calls here?
    ActiveRecord::Base.establish_connection(c)
    ActiveRecord::Tasks::DatabaseTasks.create(c)
    ActiveRecord::Base.establish_connection(c)
  • Do we need the travis dependency anymore? Got rid of it bc the Rails 7.1 gemfile test would fail with a Faraday related error, and I saw that Faraday's pulled in by travis.

@argvniyx-enroute argvniyx-enroute force-pushed the argv/main/add-rails-7-1-support branch 2 times, most recently from ba1fe88 to 2d297c7 Compare March 20, 2024 20:07
I couldn't get this to run locally, because `tasks += ...` would
complain about tasks being nil.
I think this is what's tripping CI up; we try to connect to the database
and then create it. I think we intended to only do `create` and then
`connect` (?)
I'd expect CI to show me this trace, so this might not be it, but when
trying `bin/rails db:create` locally the whole thing failed because
`config.assets...` comes from sprockets-rails, which is no longer
bundled by default 🤷. Maybe unblocking this allows us to create
the database (?)
@argvniyx-enroute argvniyx-enroute force-pushed the argv/main/add-rails-7-1-support branch 2 times, most recently from 557d831 to 84ddca0 Compare March 21, 2024 18:27
this accomplished nothing :-(
@argvniyx-enroute argvniyx-enroute force-pushed the argv/main/add-rails-7-1-support branch from 84ddca0 to 12eb529 Compare March 21, 2024 19:30
@argvniyx-enroute argvniyx-enroute force-pushed the argv/main/add-rails-7-1-support branch from 12eb529 to 97cadc0 Compare March 21, 2024 19:43
@@ -43,7 +44,7 @@ jobs:
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I was trying to figure out why things failed, I figured I might as well align this to other yml files.

@@ -32,6 +32,7 @@ if (Rails.env.development? || Rails.env.test?) && defined? Dummy

task(:default).clear
if ENV['APPRAISAL_INITIALIZED'] || ENV['CI']
tasks = [:spec]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All this time.......we weren't actually running tests in CI (!) This would fail silently afaict. I stumbled upon this by trying to run be rake locally.

Copy link
Member

Choose a reason for hiding this comment

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

😱

Comment on lines +13 to +14
username: <%= ENV['POSTGRES_USER'] %>
host: localhost
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI failed on me god knows how many times because I couldn't figure out how to configure this. This worked, but I think any combination of username/password or username POSTGRES_HOST_AUTH_METHOD=true would as long as we set host here. LMK if there's a better way to do this (since I mostly went with existing configs in other gems)

@@ -27,16 +27,16 @@
# Debug mode disables concatenation and preprocessing of assets.
# This option may cause significant delays in view rendering with a large
# number of complex assets.
config.assets.debug = true
# config.assets.debug = true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't run bundle exec rails console because rails no longer includes sprockets-rails by default, and this fails. I can get rid of these, if you'd like.

@argvniyx-enroute argvniyx-enroute marked this pull request as ready for review March 21, 2024 21:08
Copy link
Member

@smudge smudge left a comment

Choose a reason for hiding this comment

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

domain LGTM && platform LGTM!

@samandmoore
Copy link
Member

  • Why do we have two establish_connection calls here?

i think i added that code. it's strange but basically once upon a time i needed the first one to connect to the db and then the second one to reconnect after creating the database. maybe it doesn't need to happen anymore? i'm fine to try cleaning that up.

  • Do we need the travis dependency anymore? Got rid of it bc the Rails 7.1 gemfile test would fail with a Faraday related error, and I saw that Faraday's pulled in by travis.

nope! remove it!

@smudge smudge merged commit aa1da89 into main Mar 22, 2024
5 checks passed
@smudge smudge deleted the argv/main/add-rails-7-1-support branch March 22, 2024 02:09
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.

3 participants