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 5.2 #1771

Closed
wants to merge 20 commits into from

Conversation

5 participants
@thomasdziedzic
Copy link
Contributor

thomasdziedzic commented Aug 24, 2018

  • someone that knows how production works should review and merge this commit 9f74043 or tell me what needs to get merged.
@thomasdziedzic

This comment has been minimized.

Copy link
Contributor Author

thomasdziedzic commented Aug 24, 2018

I have been able get it down to a single test failure: bundle exec rails test test/functional/api/v1/rubygems_controller_test.rb -n '/with a confirmed user authenticated On POST to create for new gem/'

But I don't know why there aren't any full stack traces available for the test.

Stepping through the code, I found out that there is a scrub_env! call on the request.
https://github.com/rails/rails/blob/v5.2.1/actionpack/lib/action_controller/test_case.rb#L601-L610
This is the culprit that broke the tests.

Conclusion: Looks like rails 5.2.1 broke backwards compatibility: rails/rails#32673

thomasdziedzic added some commits Aug 24, 2018

fix rubygems api controller test
rails/rails#32673

Rails 5.2.1 broke backwards compatibility with setting RAW_POST_DATA in
a test.
Fix deprecated query methods.
DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s):
"number, platform, sha256, info_checksum, required_ruby_version, required_rubygems_version, versions.created_at, string_agg(dependencies.requirements, '@' ORDER BY rubygems_dependencies.name), string_agg(coalesce(rubygems_dependencies.name, '0'), ',' ORDER BY rubygems_dependencies.name) AS dep_name".

Non-attribute arguments will be disallowed in Rails 6.0.

This method should not be called with user-provided values, such as request parameters or model attributes.

Known-safe values can be passed by wrapping them in Arel.sql().

@thomasdziedzic thomasdziedzic changed the title Rails 5.2 [wip] Rails 5.2 Aug 24, 2018

@thomasdziedzic

This comment has been minimized.

Copy link
Contributor Author

thomasdziedzic commented Aug 24, 2018

I opened an issue to track the default configuration migration. I decided not to complete that in this pull request after discovering that the previous rails 5.0 & 5.1 migrations didn't migrate to those default configurations. I think the default configurations should be completed in order, starting with 5.0 and moving forward.

I think I'm ready for someone to do a review.

@bundlerbot

This comment has been minimized.

Copy link
Collaborator

bundlerbot commented Sep 10, 2018

☔️ The latest upstream changes (presumably 3f329e5) made this pull request unmergeable. Please resolve the merge conflicts.

@greysteil

This comment has been minimized.

Copy link
Contributor

greysteil commented Sep 13, 2018

I'm on the Bundler team, not the Rubygems team, but this looks pretty reasonable to me.

One requirement before merging will be to disable encrypted cookies (without which we wouldn't be able to roll back). See https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-1-to-rails-5-2 for details.

Because of Rubygems' vendoring setup, we'll also need a maintainer to follow the steps here when this is ready to merge. @dwradcliffe are you the right person do that?

@thomasdziedzic

This comment has been minimized.

Copy link
Contributor Author

thomasdziedzic commented Sep 13, 2018

Encrypted cookies are disabled by default. This is why the line to add them in is commented out in the new rails configuration file. https://github.com/rubygems/rubygems.org/pull/1771/files#diff-0b94a9ee8eb607505aa56304e7a6f626R20

The path to migration for new config options is documented in this issue: #1773

@greysteil

This comment has been minimized.

Copy link
Contributor

greysteil commented Sep 13, 2018

You're quite right - docs confirm it here.

OK, one thing that would be helpful is to get these commits squashed.

production:
adapter: redis
url: <%= ENV.fetch("REDIS_URL") { "redis://localhost:6379/1" } %>
channel_prefix: gemcutter_production

This comment has been minimized.

Copy link
@dwradcliffe

dwradcliffe Sep 13, 2018

Member

We don't use actioncable, do we need this file? Better to remove and keep things simple or leave it because it's vanilla rails?

@dwradcliffe dwradcliffe requested a review from sonalkr132 Sep 13, 2018

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Overall looks good to me. @sonalkr132 or I can update the vendored gems when you're ready.

sonalkr132 added a commit that referenced this pull request Oct 21, 2018

@sonalkr132

This comment has been minimized.

Copy link
Member

sonalkr132 commented Oct 21, 2018

Merged as 3538e87. Thanks @thomasdziedzic ❤️ 💛 💚 💙

Better to remove and keep things simple or leave it because it's vanilla rails?

Removed on master.

@sonalkr132 sonalkr132 closed this Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.