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

Call client.config instead of client #1640

Merged
merged 5 commits into from
Nov 17, 2022

Conversation

kaylareopelle
Copy link
Contributor

Overview

In some cases, the RedisClient object cannot directly access methods like db, port, or path. These methods are always available on the client.config object.

Resolves: #1639

Submitter Checklist:

  • Include a link to the related GitHub issue, if applicable
  • Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

  • Perform code review
  • Add performance label
  • Perform appropriate level of performance testing
  • Confirm all checks passed
  • Add version label prior to acceptance

In some cases, the client object cannot directly access methods
like db, port, or path. These methods are always available on the
 client.config object.
@fallwith fallwith force-pushed the bugfix/redisclient_undefined_method_db branch from a15a5ea to 9d4d175 Compare November 17, 2022 04:11
added a regression test for #1639 that reproduces the issue of being
unable to call `(self.)client.db` when `(self.)client` returns an
instance of `RedisClient` and not `Redis::Client`
fallwith
fallwith previously approved these changes Nov 17, 2022
sidekiq_with_redis_test.rb Outdated Show resolved Hide resolved
sidekiq_with_redis_test.rb Outdated Show resolved Hide resolved
place the Sidekiq/Redis test class in its proper location
@github-actions
Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.31% 93%
Branch 84.28% 84%

improve the skipping logic

- don't perform a Ruby version check to artificially limit the number of
  environments that will conduct the test. the test is not Ruby version
  dependent, but is also not particularly slow. let it run everywhere.
- make the skip logic more clear, both in how the code itself flows and
  the reasoning behind it as it pertains to informing readers about the
  reproduction of the bug
tannalynn
tannalynn previously approved these changes Nov 17, 2022
Copy link
Contributor

@tannalynn tannalynn left a comment

Choose a reason for hiding this comment

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

Thanks for getting more detailed about when this runs!

update the v8.13.0 changelog entry to point Sidekiq v7.0 users to
v8.13.1 of the agent

[skip ci]
@fallwith fallwith merged commit f926d6e into dev Nov 17, 2022
@fallwith
Copy link
Contributor

reminder: this bugfix branch has been mentioned publicly and should not be deleted until after v8.13.1 is released

@hannahramadan hannahramadan deleted the bugfix/redisclient_undefined_method_db branch November 21, 2022 19:39
@hannahramadan hannahramadan restored the bugfix/redisclient_undefined_method_db branch November 21, 2022 19:39
@kaylareopelle kaylareopelle deleted the bugfix/redisclient_undefined_method_db branch December 7, 2022 19:49
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.

NoMethod error raised from redis instrumentation when queueing ActiveJob sidekiq job
3 participants