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

Resolve sidekiq warning #20805

Merged

Conversation

medwards1771
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

On running /bin/startup, I noticed the warning:
"WARN: config.options[:key] = value is deprecated, use config[:key] = value"

pointing to forem/config/initializers/sidekiq.rb:29.

This commit resolves that warning by removing .options

QA Instructions, Screenshots, Recordings

Run the bin/startup script locally from forem/forem:main. Notice the warning described above.
Run the bin/startup script locally from medwards1771/forem:resolve-sidekiq-warning-on-startup (or just make the change on your local copy of forem). Notice the warning disappear.

Added/updated tests?

We encourage you to keep the code coverage percentage at 80% and above.

  • Yes
  • No, and this is why: I'm guessing this configuration detail is not tested. Also, I got some ActiveRecord::NoDatabaseError errors when attempting to run the test suite locally with bin/rspec since I don't have the database "Forem_test" and figured it would be faster to see the test suite run via CI.
  • I need help with writing tests

On running ./bin/startup, I noticed the warning:
"WARN: `config.options[:key] = value` is deprecated, use `config[:key] = value`"

pointing to forem/config/initializers/sidekiq.rb:29.

This commit resolves that warning by removing `.options`
@medwards1771 medwards1771 requested review from a team as code owners March 28, 2024 13:53
@medwards1771 medwards1771 requested review from maestromac and lightalloy and removed request for a team March 28, 2024 13:53
Copy link
Contributor

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

how did I missed this. Nice catch!

@@ -26,7 +26,7 @@ def not_past_scheduled_time?(current_time)
# https://github.com/ondrejbartas/sidekiq-cron/issues/254
# Sidekiq default is 5, we don't need it quite that often but would like it more than
# every 30 seconds which the gem defaults to
Sidekiq.options[:poll_interval] = 10
Sidekiq[:poll_interval] = 10
Copy link
Member

Choose a reason for hiding this comment

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

actually I think it would be better with

Suggested change
Sidekiq[:poll_interval] = 10
config[:poll_interval] = 10

since it's already in this block and suggested by the dep warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good callout, ty. will update.

Sidekiq is set up in the block to be referenced as "config", see:

  Sidekiq.configure_server do |config|
Copy link
Contributor

github-actions bot commented Mar 28, 2024

Uffizzi Preview deployment-49096 was deleted.

@maestromac maestromac merged commit 2c0c446 into forem:main Mar 28, 2024
28 of 30 checks passed
@medwards1771 medwards1771 deleted the resolve-sidekiq-warning-on-startup branch March 28, 2024 14:20
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.

None yet

2 participants