Skip to content

Conversation

@wwwfeng
Copy link
Contributor

@wwwfeng wwwfeng commented Mar 21, 2025

Currently, there are pub commands, but no sub commands, which feels odd. This PR adds the missing sub functionality.

@kevwan kevwan requested a review from Copilot March 24, 2025 16:27
@kevwan kevwan force-pushed the feat/redis_pubsub branch from b1aeec4 to f328d98 Compare March 24, 2025 16:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds subscription functionality for Redis by introducing new cluster and client managers for pub/sub as well as corresponding API methods and tests.

  • Introduces getPubSubCluster and getPubSubClient functions for handling Redis subscriptions.
  • Adds new subscription methods (Subscribe, SubscribeCtx, PSubscribe, PSubscribeCtx) in redis.go.
  • Provides tests to validate the new pub/sub behavior.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
core/stores/redis/redispubsubclustermanager.go Adds cluster client creation for pub/sub subscriptions
core/stores/redis/redispubsubclientmanager.go Adds standard client creation for pub/sub subscriptions
core/stores/redis/redis_test.go Implements tests for different pub/sub subscription scenarios
core/stores/redis/redis.go Adds new subscription API methods and introduces getPubSubRedis helper

@codecov
Copy link

codecov bot commented Mar 24, 2025

Codecov Report

Attention: Patch coverage is 35.92233% with 66 lines in your changes missing coverage. Please review.

Project coverage is 94.23%. Comparing base (8690859) to head (f328d98).
Report is 289 commits behind head on master.

Files with missing lines Patch % Lines
core/stores/redis/redispubsubclientmanager.go 0.00% 36 Missing ⚠️
core/stores/redis/redis.go 40.62% 16 Missing and 3 partials ⚠️
core/stores/redis/redispubsubclustermanager.go 68.57% 8 Missing and 3 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
core/stores/redis/redispubsubclustermanager.go 68.57% <68.57%> (ø)
core/stores/redis/redis.go 98.22% <40.62%> (-1.55%) ⬇️
core/stores/redis/redispubsubclientmanager.go 0.00% <0.00%> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

var tlsConfig *tls.Config
if r.tls {
tlsConfig = &tls.Config{
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.
var tlsConfig *tls.Config
if r.tls {
tlsConfig = &tls.Config{
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.
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.

1 participant