-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(input.redis_sentinel): fix sentinel and replica stats gathering #12229
fix(input.redis_sentinel): fix sentinel and replica stats gathering #12229
Conversation
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.
Thank you for revamping this! One comment in line. Ideally you would continue to use the testutil.Container
function, so I have on place to change things when testcontainers breaks backwards comparability again. However, I think this should be ok.
WaitingFor: wait.ForAll( | ||
wait.ForExposedPort(), | ||
), |
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.
Can you also have this wait for the log message "Ready to accept connections" as well? I like to see both a port + log. Similar to what you already do for the sentinels.
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 added the wrapper functions in order to gain readability as I felt things were getting a bit hectic with bootstrapping three containers for this test case. |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Looks good to me. Thanks for the fix @spideyfusion!
Required for all PRs
resolves #12152
I improved the integration test by making sure the plugin gets Initialized and talks to an actual Redis Sentinel instance.