-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: dependency upgrades #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshmcclain I feel like I've spent too long trying to get this up and running properly. Can we pair on this during our 1-1 on Monday?
@@ -13,13 +13,13 @@ gem 'webpacker', '~> 4.0' | |||
gem 'turbolinks', '~> 5' | |||
gem 'bootsnap', '>= 1.4.2', require: false | |||
gem 'activeadmin' | |||
gem 'devise', github: 'heartcombo/devise', branch: 'ca-omniauth-2' | |||
gem 'devise' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the ca-omniauth-2
branch doesn't exist anymore and was merged into main
for versions 4.8 and later heartcombo/devise#5327 (comment)
|
||
PLATFORMS | ||
ruby | ||
arm64-darwin-23 | ||
x86_64-linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 not sure this change is ok but bundle lock --add-platform ruby
didn't work as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshmcclain this is the main concern. Adding x86_64-linux
allows the Heroku integration tests to pass. Is that sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that should be sufficient. There's some history at play here: older versions of bundler used to just include ruby
as the default fallback platform, but newer versions started to list the actual platform targets explicitly. So the way you have it should be fine. It's the more up-to-date way of specifying platforms in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And I see below that you bundled with 2.4.22, so this is expected)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great! Alicia and I were wondering about this on Cocoon, too.
@josephineweidner yes absolutely! Can you qualify "properly"? Tests in CI look okay -- is there an error you're hitting locally? |
@joshmcclain yes! Responded inline in the comment above! |
@joshmcclain this should be ready for approval and merge in that case! Thanks for your review 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
These changes were necessary to get this repo running on my Mac M1. This PR also adds the
active_model_serializers
gem (necessary for #32).Will leave comments throughout and mark as draft until I'm confident in this being merged