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
Upgrade to Rails 7 #5762
Upgrade to Rails 7 #5762
Conversation
1c0dc32
to
dc29e91
Compare
dc29e91
to
e7b97fc
Compare
e7b97fc
to
8d8d294
Compare
c608c4f
to
9db6bca
Compare
cd552d9
to
d6c9a74
Compare
830059e
to
f4762ee
Compare
ec072da
to
e60e0c5
Compare
e60e0c5
to
e89e062
Compare
e89e062
to
1637868
Compare
2d702a5
to
a437d73
Compare
a729d9b
to
35aba3d
Compare
35aba3d
to
0568c0f
Compare
0568c0f
to
f2bc030
Compare
@@ -62,8 +62,6 @@ lint: ## Runs all lint tests | |||
make lint_analytics_events | |||
@echo "--- brakeman ---" | |||
bundle exec brakeman | |||
@echo "--- zeitwerk check ---" |
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.
This is no longer needed since the classic
loader no longer exists in Rails 7
@@ -172,6 +172,7 @@ | |||
stub_analytics | |||
stub_sign_in(user) | |||
allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') | |||
request.host = 'localhost:3000' |
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.
This is required to ensure that the webauthn domain name does not conflict with the host
in the request, otherwise we get (and also because this isn't a request where we want to allow other hosts):
ActionController::Redirecting::UnsafeRedirectError:
Unsafe redirect to "http://localhost/account", pass allow_other_host: true to redirect anyway.
@@ -43,9 +43,9 @@ | |||
'script[type="application/json"][data-asset-map]', | |||
visible: :all, | |||
text: { | |||
'clock.svg' => '/clock.svg', | |||
'clock.svg' => 'http://test.host/clock.svg', |
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.
This is due to request
now existing when the function is called in this spec here:
identity-idp/app/helpers/script_helper.rb
Lines 78 to 80 in f2bc030
elsif request | |
request.base_url | |
end |
I'm not sure why it would have changed, but I don't think it affects the primary motivation behind the test?
# Generate CSRF tokens that are encoded in URL-safe Base64. | ||
# | ||
# This change is not backwards compatible with earlier Rails versions. | ||
# It's best enabled when your entire app is migrated and stable on 6.1. |
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.
We have been stable on 6.1 for some time now 🙂
only_option = if_option_for(action)[0] | ||
|
||
"#{only_option.class}OptionParser".constantize.new(only_option).parse | ||
if_option_for(action)[0] |
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.
I'll be honest, I don't have confidence in my understanding of this file's metaprogramming, and I'd rather see us test the behaviors around the before_actions
via controller specs than ensure a method is called.
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.
omg I didn't realize this file was parsing the source, and not just introspecting the controller in memory. but +1 would rather see something like expect(controller).to receive(:some_action).and_call_original
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.
It's passing around a bunch of stuff and has to call instance_variable_get
to get private instance variables, which isn't ideal and it's hard to follow the process. I don't mind it as kind of a simpler check if it was well-supported, but it's unfortunately not the case.
It is used pretty extensively though.
2b86d9a
to
a635f27
Compare
a635f27
to
8c2b310
Compare
8c2b310
to
f2cc0c3
Compare
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.
LGTM!
changelog: Internal, Dependencies, Update to Rails 7
f2cc0c3
to
8a17616
Compare
This is more or less ready to go with the exception of rspec supporting Rails 7: