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

chore: use correct synatx for rack-timeout for ruby 3 #395

Closed
wants to merge 7 commits into from

Conversation

ghassanmas
Copy link
Member

@ghassanmas ghassanmas commented Oct 11, 2022

In order to resolve the following error:

tutor_nightly_local-forum-1                      | config.ru:18:in `block in <main>': undefined method `timeout=' for Rack::Timeout:Class (NoMethodError)
tutor_nightly_local-forum-1                      | 	from /app/cs_comments_service/vendor/bundle/ruby/3.0.0/gems/rack-1.6.13/lib/rack/builder.rb:55:in `instance_eval'
tutor_nightly_local-forum-1                      | 	from /app/cs_comments_service/vendor/bundle/ruby/3.0.0/gems/rack-1.6.13/lib/rack/builder.rb:55:in `initialize'

I have to change config.ru file following the documenation at https://github.com/zombocom/rack-timeout#sinatra-and-other-rack-apps
I couldn't change using the doc, I got an error execption, so I only way I was able to change it is to use ENV varaible.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 11, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @ghassanmas! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mtyaka
Copy link
Contributor

mtyaka commented Nov 9, 2022

@ghassanmas Thanks for catching and fixing this issue! @xitij2000 recently finalised his ruby3 PR -- can you rebase and address my comment above?

@ghassanmas
Copy link
Member Author

@mtyaka the rebase is done, though I couldn't see your comments.

@@ -12,10 +12,6 @@

puts "Loading config.ru."

require "rack-timeout"
use Rack::Timeout # Call as early as possible so rack-timeout runs before other middleware.
Rack::Timeout.timeout = 20
Copy link
Contributor

Choose a reason for hiding this comment

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

@ghassanmas I believe removing all of this code completely disables rack-timeout. According to the docs, we should keep these two lines:

require "rack-timeout"
use Rack::Timeout

and only remove the last one. That will initialise rack-timeout with a timeout value from the RACK_TIMEOUT_SERVICE_TIMEOUT environment variable. If the environment variable is not present, it will default to 15 seconds.

@mtyaka
Copy link
Contributor

mtyaka commented Nov 9, 2022

Sorry about that @ghassanmas, I forgot to publish the comment, but I published it now.
Thanks for rebasing!

    Also update readmde about how to update the rack-timeout
@ghassanmas
Copy link
Member Author

@mtyaka I resolved your comment and pushed, but I am still testing it... I had to push it so bceause the test enviroment can pick up that commit given its not same as my local...
Anyway I will repot soon, the test will also be based with rebased i.e. the latest of @xitij2000 commits

@ghassanmas
Copy link
Member Author

Tested all good!.

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 96.08% // Head: 96.06% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (62eb703) compared to base (712a8bd).
Patch has no changes to coverable lines.

❗ Current head 62eb703 differs from pull request most recent head eeb461e. Consider uploading reports for the commit eeb461e to get more accurate results

Additional details and impacted files
@@                Coverage Diff                @@
##           kshitij/ruby3     #395      +/-   ##
=================================================
- Coverage          96.08%   96.06%   -0.03%     
=================================================
  Files                 58       58              
  Lines               4522     4469      -53     
=================================================
- Hits                4345     4293      -52     
+ Misses               177      176       -1     
Impacted Files Coverage Δ
api/notifications_and_subscriptions.rb 87.50% <0.00%> (-1.39%) ⬇️
spec/support/matchers.rb 77.77% <0.00%> (-1.17%) ⬇️
app.rb 88.78% <0.00%> (-0.51%) ⬇️
models/concerns/searchable.rb 84.84% <0.00%> (-0.45%) ⬇️
api/comment_threads.rb 88.52% <0.00%> (-0.37%) ⬇️
models/subscription.rb 94.11% <0.00%> (-0.33%) ⬇️
models/comment.rb 87.60% <0.00%> (-0.30%) ⬇️
lib/task_helpers.rb 80.14% <0.00%> (-0.29%) ⬇️
api/comments.rb 92.42% <0.00%> (-0.23%) ⬇️
presenters/thread_utils.rb 97.22% <0.00%> (-0.22%) ⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xitij2000
Copy link
Contributor

Tested all good!.

I've incorporated this commit into my ruby 3 upgrade PR since without that the forum service is broken on unicorn.

@openedx-webhooks
Copy link

@ghassanmas Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants