feat(multi-tenant): canonical lib-commons v4.5 tenant-manager integration#72
Closed
jeffersonrodrigues92 wants to merge 5 commits intodevelopfrom
Closed
feat(multi-tenant): canonical lib-commons v4.5 tenant-manager integration#72jeffersonrodrigues92 wants to merge 5 commits intodevelopfrom
jeffersonrodrigues92 wants to merge 5 commits intodevelopfrom
Conversation
… RabbitMQ Implement multi-tenant support using lib-commons v3 tenant-manager sub-packages: - Bootstrap: TenantManagerComponents with PostgresManager, RabbitMQManager, TenantMiddleware, circuit breaker, and conditional initialization (MULTI_TENANT_ENABLED flag) - Redis: replace manual auth.GetTenantID prefix with valkey.GetKeyFromContext for dedupe and cache services - S3: replace manual getTenantPrefixedKey with tms3.GetObjectStorageKeyForTenant for export storage - RabbitMQ: per-tenant vhost isolation via tmrabbitmq.Manager (Layer 1) and X-Tenant-ID audit headers (Layer 2) with dual-mode publishers - Metrics: 4 canonical tenant metrics (connections, errors, consumers, messages) with no-op variant for single-tenant mode - Config: 7 canonical MULTI_TENANT_* environment variables with validation - Backward compat: MULTI_TENANT_ENABLED=false preserves exact single-tenant behavior X-Lerian-Ref: 0x1
Add docs/multi-tenant-guide.md with components table, 7 canonical env vars, activation/deactivation steps, migration notes, common errors, and architecture overview. X-Lerian-Ref: 0x1
- Add nil channel guard in publishMultiTenant for both ingestion and matching RabbitMQ publishers to prevent panic on nil *amqp.Channel - Require AUTH_ENABLED=true when MULTI_TENANT_ENABLED=true to prevent silent loss of tenant isolation - Update bootstrap README with current function signatures
WalkthroughThis PR adds multi-tenant runtime wiring and operational changes: tenant-aware key helpers (valkey.GetKeyContext, tms3.GetS3KeyStorageContext), tenant-cache-backed event discovery over Redis Pub/Sub, tenant-prefixed S3 keys, and nil-channel checks for RabbitMQ tenant channels. It introduces TenantMetrics (OpenTelemetry), a RabbitMQMultiTenantManager interface, middleware ordering clarification, config keys for multi-tenant Redis, and related unit tests and docs updates. go.mod bumps github.com/LerianStudio/lib-commons/v4 to v4.6.0. Sequence Diagram(s)mermaid mermaid 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
github-actions
bot
added
area: dependencies
area: internal
area: tests
area: documentation
and removed
size/XL
labels
Contributor
|
This PR is very large (24 files, 1698 lines changed). Consider breaking it into smaller PRs for easier review. |
Contributor
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/matching/adapters/rabbitmq/event_publisher.go (1)
257-261:⚠️ Potential issue | 🔴 CriticalClosing the channel violates the
RabbitMQMultiTenantManagerinterface contract.Same issue as the ingestion publisher. The interface at
internal/shared/adapters/rabbitmq/multi_tenant.godocuments that callers should NOT close channels as they are pool-managed. This defer block needs to be removed or the interface contract updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/matching/adapters/rabbitmq/event_publisher.go` around lines 257 - 261, The defer that closes the tenant channel in event_publisher.go violates the RabbitMQMultiTenantManager contract (channels are pool-managed and must not be closed by callers); remove the defer closure block that calls ch.Close() (and any related error wrapping logic) from the publish flow in the EventPublisher implementation so the channel is left to the multi-tenant manager, and mirror the same removal pattern used for the ingestion publisher to keep behavior consistent with the interface RabbitMQMultiTenantManager.internal/ingestion/adapters/rabbitmq/event_publisher.go (1)
181-185:⚠️ Potential issue | 🔴 CriticalRemove
defer ch.Close()from multi-tenant event publishers—violates interface contract.The
RabbitMQMultiTenantManagerinterface (internal/shared/adapters/rabbitmq/multi_tenant.go, lines 13-14) explicitly states: "The caller should NOT close the channel; it is managed by the pool." Thedefer ch.Close()at lines 181-185 violates this contract.This issue affects both ingestion and matching contexts (matching/adapters/rabbitmq/event_publisher.go lines 257-260 has the same violation). Remove the deferred close call from both publishers to comply with the interface contract and prevent double-close panics and pool corruption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ingestion/adapters/rabbitmq/event_publisher.go` around lines 181 - 185, The deferred channel close in the publisher (the defer that calls ch.Close() inside internal/ingestion/adapters/rabbitmq/event_publisher.go) violates the RabbitMQMultiTenantManager contract ("caller should NOT close the channel; it is managed by the pool"); remove that defer entirely so the channel is not closed by the publisher (also mirror the same removal in the matching publisher file matching/adapters/rabbitmq/event_publisher.go). Keep all other error handling intact and rely on the pool/manager to close channels per the RabbitMQMultiTenantManager interface.internal/reporting/adapters/storage/s3_client.go (1)
201-220:⚠️ Potential issue | 🟠 MajorAdd structured logging fields instead of using
fmt.Sprintffor error visibility across tenants.The error logging in these methods should use structured fields instead of concatenating the key into a single message string. Change from:
logger.Log(ctx, libLog.LevelError, fmt.Sprintf("failed to upload object %s: %v", key, err))To:
logger.Log(ctx, libLog.LevelError, "failed to upload object", libLog.String("key", key), libLog.String("error", err.Error()))This enables better observability—errors across different tenants and keys become distinguishable in structured logs and monitoring dashboards.
Also applies to: 255-283, 307-329, 351-367, 395-414, 436-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/reporting/adapters/storage/s3_client.go` around lines 201 - 220, Replace the unstructured error log that uses fmt.Sprintf in the S3 upload/error paths with structured logging fields: when handling errors after getTenantPrefixedKey or client.s3.PutObject (references: getTenantPrefixedKey, client.s3.PutObject, libOpentelemetry.HandleSpanError), call logger.Log(ctx, libLog.LevelError, "failed to upload object", libLog.String("key", key), libLog.String("error", err.Error())) instead of fmt.Sprintf; apply the same structured-field pattern to the other similar error sites mentioned (lines 255-283, 307-329, 351-367, 395-414, 436-462) so each logger.Log includes distinct libLog.String fields for "key" and "error".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/multi-tenant-guide.md`:
- Around line 20-34: The docs omit that multi-tenant mode requires auth; update
the env table and the activation example to include AUTH_ENABLED (e.g.,
`AUTH_ENABLED` | bool | `true` | No | Authentication must be enabled for
multi-tenant) and add `export AUTH_ENABLED=true` to the example block so that
MULTI_TENANT_ENABLED/MULTI_TENANT_URL/MULTI_TENANT_ENVIRONMENT will not cause
startup validation to fail; mention in the same section that multi-tenant
hard-fails if AUTH_ENABLED is false.
In `@internal/bootstrap/routes_test.go`:
- Around line 738-749: The test TestBuildProtectedMiddlewares_AllMiddlewares
only checks the slice length but not execution order; update the test to verify
that buildProtectedMiddlewares returns middlewares in the expected sequence by
executing them and asserting the observed execution order is [tenantDB,
rateLimiter, idempotency]; instrument the idempotency, rateLimiter, and tenantDB
handlers to append identifiers to an executionOrder slice, wire result[0],
result[1], result[2] into a temporary Fiber route, invoke it with
httptest.NewRequest, and assert the executionOrder equals the expected slice.
In `@internal/ingestion/adapters/rabbitmq/event_publisher_test.go`:
- Around line 125-132: The test is calling publishMultiTenant with the wrong
parameter order and extra nils; update the call to match the production
signature publishMultiTenant(ctx, tenantID, exchange, routingKey, msg) by
passing context.Background(), the tenant ID ("" for empty), an exchange (use ""
if none), the routing key "test.routing.key", and a single amqp.Publishing value
(remove the two nil arguments) so the call has exactly five arguments in the
correct order.
- Around line 38-39: The test asserts use the wrong field name: update the
assertions in event_publisher_test.go to reference the production struct's field
rmqManager instead of rabbitmqManager (e.g., change publisher.rabbitmqManager to
publisher.rmqManager) so the test compiles against the EventPublisher struct;
keep the confirmablePublisher assertion as-is and run tests to verify.
- Around line 105-113: The test uses the wrong field name: update assertions in
Test (where you construct NewEventPublisherMultiTenant with
mockRabbitMQMultiTenantManager) to reference publisher.rmqManager instead of
publisher.rabbitmqManager; change the NotNil assertion to assert.NotNil(t,
publisher.rmqManager) and keep the confirmablePublisher check as assert.Nil(t,
publisher.confirmablePublisher) so the test aligns with the
EventPublisherMultiTenant struct's actual field names.
- Around line 148-155: The call to publishMultiTenant in the test uses the wrong
parameter order and extra arguments; update the call to match the
publishMultiTenant function signature (reorder arguments and remove or add
parameters to match the actual signature used in the implementation),
referencing the publishMultiTenant function and the test invocation in
event_publisher_test.go so the context, routing key, tenant ID, amqp.Publishing
and any options are passed in the exact order and count expected by the
function.
In `@internal/ingestion/adapters/rabbitmq/event_publisher.go`:
- Around line 177-179: Define a sentinel error var named errNilChannelReturned
(e.g. var errNilChannelReturned = errors.New("nil channel returned for tenant"))
near the top of the file, then replace the dynamic error return in the ch == nil
check with a wrapped sentinel: return fmt.Errorf("get tenant channel: %w for
tenant %s", errNilChannelReturned, tenantID), so the code uses the
errNilChannelReturned sentinel and wraps it with the tenantID context; ensure
the errors package is imported if not already.
In `@internal/matching/adapters/rabbitmq/event_publisher_test.go`:
- Around line 39-40: The test uses a wrong struct field name — update the
assertions in the test to use the real production field names: replace
references to publisher.rabbitmqManager with publisher.rmqManager so they match
the struct in event_publisher.go; keep the check for
publisher.confirmablePublisher as-is (or adjust only if the production field
name differs) so the test compiles against the actual EventPublisher fields.
- Around line 127-135: The test calls publisher.publishMultiTenant with the
wrong arity and ordering which will not compile; update the call in
event_publisher_test.go to match the production signature
publishMultiTenant(ctx, tenantID, exchange, routingKey, msg) by passing
context.Background(), the tenant ID (non-empty or as intended), the exchange
string, the routing key string ("test.routing.key"), and an amqp.Publishing as
the msg, and remove the extra nil, nil, "match-123" arguments (or move their
data into the amqp.Publishing if they represent message fields).
- Around line 151-159: The test call to publishMultiTenant in
event_publisher_test.go uses the wrong parameter order/count; update the
publisher.publishMultiTenant(...) invocation to match the actual function
signature of publishMultiTenant (reorder arguments and add/remove any missing
parameters such as tenant ID, routing key, amqp.Publishing, headers/properties,
context/correlation ID) so the types and sequence align with the
publishMultiTenant method definition; use the function name publishMultiTenant
as the reference and ensure the test supplies exactly the same arguments in the
same order as the implementation.
In `@internal/matching/adapters/rabbitmq/event_publisher.go`:
- Around line 253-255: Define a package-level sentinel error named
errNilChannelReturned (errors.New("nil channel returned for tenant")) and
replace the current nil-channel return in event_publisher.go so it wraps that
sentinel and includes the tenant ID (use fmt.Errorf with %w to wrap
errNilChannelReturned and include tenantID), referencing the existing ch and
tenantID variables so callers can use errors.Is(err, errNilChannelReturned) to
detect this case.
In `@internal/reporting/adapters/storage/s3_client.go`:
- Around line 58-65: Update the doc comment for getTenantPrefixedKey to
reference the current API name tms3.GetS3KeyStorageContext (replace the old
s3.GetObjectStorageKeyForTenant mention) and modify the function to wrap any
error returned from tms3.GetS3KeyStorageContext using fmt.Errorf with the %w
verb before returning; keep the behavior of stripping leading slashes and the
tenant-prefix logic but ensure errors are wrapped for proper propagation.
In `@internal/shared/infrastructure/metrics/tenant_metrics_test.go`:
- Around line 139-152: The test
TestNewTenantMetricsFromConfig_MultiTenantEnabled_ReturnsRealMetrics calls the
singleton-using NewTenantMetricsFromConfig(true) and is marked t.Parallel(),
creating a hidden order dependency with TestNewTenantMetrics_SingletonBehavior;
either remove the t.Parallel() call from
TestNewTenantMetricsFromConfig_MultiTenantEnabled_ReturnsRealMetrics to ensure
deterministic initialization of the singleton, or if you prefer to keep it, add
a clear comment above the test referencing NewTenantMetricsFromConfig and the
singleton behavior stating why parallelism is safe (global otel.Meter defaults
to noop) so future maintainers understand the dependency.
In `@internal/shared/infrastructure/tenant/adapters/multi_tenant_test.go`:
- Around line 144-147: The test currently binds defaultTenantSlug to the
compile-time constant auth.DefaultTenantSlug which can mismatch runtime
defaults; instead obtain the effective default at runtime by calling the public
accessor auth.GetTenantSlug (or the proper exported function that returns the
current tenant slug) and use that value in the assertion (replace
auth.DefaultTenantSlug / defaultTenantSlug with the result of
auth.GetTenantSlug) so the test uses the same fallback logic as
getDefaultTenantSlug().
- Line 206: Replace the raw string key used in context.WithValue when creating
childCtx: instead of context.WithValue(parentCtx, "custom_key", "custom_value")
define and use a dedicated typed key (e.g., type ctxKey string and a const like
customKey ctxKey = "custom_key") or reuse an existing auth.*Key type if
appropriate, then call context.WithValue(parentCtx, customKey, "custom_value");
update references to the key accordingly to match the pattern used by
auth.TenantIDKey/auth.TenantSlugKey and avoid collisions.
---
Outside diff comments:
In `@internal/ingestion/adapters/rabbitmq/event_publisher.go`:
- Around line 181-185: The deferred channel close in the publisher (the defer
that calls ch.Close() inside
internal/ingestion/adapters/rabbitmq/event_publisher.go) violates the
RabbitMQMultiTenantManager contract ("caller should NOT close the channel; it is
managed by the pool"); remove that defer entirely so the channel is not closed
by the publisher (also mirror the same removal in the matching publisher file
matching/adapters/rabbitmq/event_publisher.go). Keep all other error handling
intact and rely on the pool/manager to close channels per the
RabbitMQMultiTenantManager interface.
In `@internal/matching/adapters/rabbitmq/event_publisher.go`:
- Around line 257-261: The defer that closes the tenant channel in
event_publisher.go violates the RabbitMQMultiTenantManager contract (channels
are pool-managed and must not be closed by callers); remove the defer closure
block that calls ch.Close() (and any related error wrapping logic) from the
publish flow in the EventPublisher implementation so the channel is left to the
multi-tenant manager, and mirror the same removal pattern used for the ingestion
publisher to keep behavior consistent with the interface
RabbitMQMultiTenantManager.
In `@internal/reporting/adapters/storage/s3_client.go`:
- Around line 201-220: Replace the unstructured error log that uses fmt.Sprintf
in the S3 upload/error paths with structured logging fields: when handling
errors after getTenantPrefixedKey or client.s3.PutObject (references:
getTenantPrefixedKey, client.s3.PutObject, libOpentelemetry.HandleSpanError),
call logger.Log(ctx, libLog.LevelError, "failed to upload object",
libLog.String("key", key), libLog.String("error", err.Error())) instead of
fmt.Sprintf; apply the same structured-field pattern to the other similar error
sites mentioned (lines 255-283, 307-329, 351-367, 395-414, 436-462) so each
logger.Log includes distinct libLog.String fields for "key" and "error".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c208cf63-0082-4307-a67d-31eefbf3f953
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
docs/multi-tenant-guide.mddocs/ring-dev-multi-tenant/current-cycle.jsongo.modinternal/bootstrap/README.mdinternal/bootstrap/config_validation.gointernal/bootstrap/dynamic_infrastructure_provider.gointernal/bootstrap/init.gointernal/bootstrap/multi_tenant_isolation_test.gointernal/bootstrap/routes_test.gointernal/exception/adapters/redis/callback_rate_limiter.gointernal/ingestion/adapters/rabbitmq/event_publisher.gointernal/ingestion/adapters/rabbitmq/event_publisher_test.gointernal/ingestion/adapters/redis/dedupe_service.gointernal/matching/adapters/rabbitmq/event_publisher.gointernal/matching/adapters/rabbitmq/event_publisher_test.gointernal/reporting/adapters/redis/cache_service.gointernal/reporting/adapters/storage/s3_client.gointernal/reporting/adapters/storage/s3_client_coverage_test.gointernal/shared/adapters/rabbitmq/multi_tenant.gointernal/shared/adapters/rabbitmq/multi_tenant_test.gointernal/shared/infrastructure/metrics/tenant_metrics.gointernal/shared/infrastructure/metrics/tenant_metrics_test.gointernal/shared/infrastructure/tenant/adapters/multi_tenant_test.go
Comment on lines
+20
to
+34
| | `MULTI_TENANT_ENABLED` | bool | `false` | No | Master switch for multi-tenant mode | | ||
| | `MULTI_TENANT_URL` | string | — | When enabled | Tenant Manager API URL | | ||
| | `MULTI_TENANT_ENVIRONMENT` | string | `staging` | No | Environment name for Tenant Manager | | ||
| | `MULTI_TENANT_MAX_TENANT_POOLS` | int | `100` | No | Max PostgreSQL connection pools | | ||
| | `MULTI_TENANT_IDLE_TIMEOUT_SEC` | int | `300` | No | Idle pool eviction timeout (seconds) | | ||
| | `MULTI_TENANT_CIRCUIT_BREAKER_THRESHOLD` | int | `5` | No | Circuit breaker failure threshold | | ||
| | `MULTI_TENANT_CIRCUIT_BREAKER_TIMEOUT_SEC` | int | `30` | No | Circuit breaker recovery timeout (seconds) | | ||
|
|
||
| ## 3. How to Activate | ||
|
|
||
| ```bash | ||
| export MULTI_TENANT_ENABLED=true | ||
| export MULTI_TENANT_URL=http://tenant-manager:8080 | ||
| export MULTI_TENANT_ENVIRONMENT=production | ||
| ``` |
There was a problem hiding this comment.
Document AUTH_ENABLED=true in the activation flow.
Multi-tenant mode now hard-fails when auth is disabled, but the env table and the example export block omit that toggle. Following this guide as written will fail during startup validation.
Suggested doc update
export MULTI_TENANT_ENABLED=true
+export AUTH_ENABLED=true
export MULTI_TENANT_URL=http://tenant-manager:8080
export MULTI_TENANT_ENVIRONMENT=production🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/multi-tenant-guide.md` around lines 20 - 34, The docs omit that
multi-tenant mode requires auth; update the env table and the activation example
to include AUTH_ENABLED (e.g., `AUTH_ENABLED` | bool | `true` | No |
Authentication must be enabled for multi-tenant) and add `export
AUTH_ENABLED=true` to the example block so that
MULTI_TENANT_ENABLED/MULTI_TENANT_URL/MULTI_TENANT_ENVIRONMENT will not cause
startup validation to fail; mention in the same section that multi-tenant
hard-fails if AUTH_ENABLED is false.
Comment on lines
+738
to
+749
| func TestBuildProtectedMiddlewares_AllMiddlewares(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| idempotency := func(c *fiber.Ctx) error { return c.Next() } | ||
| rateLimiter := func(c *fiber.Ctx) error { return c.Next() } | ||
| tenantDB := func(c *fiber.Ctx) error { return c.Next() } | ||
|
|
||
| result := buildProtectedMiddlewares(idempotency, rateLimiter, tenantDB) | ||
|
|
||
| // Should have tenantDB + idempotency + rateLimiter | ||
| assert.Len(t, result, 3) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying middleware ordering, not just count.
The tests validate slice length but not middleware execution order. Since tenant DB middleware must run before idempotency (to set tenant context), consider adding an assertion that verifies the order matches the expected sequence: [tenantDB, rateLimiter, idempotency].
💡 Optional: Add order verification
func TestBuildProtectedMiddlewares_AllMiddlewares_Order(t *testing.T) {
t.Parallel()
var executionOrder []string
idempotency := func(c *fiber.Ctx) error { executionOrder = append(executionOrder, "idempotency"); return c.Next() }
rateLimiter := func(c *fiber.Ctx) error { executionOrder = append(executionOrder, "rateLimiter"); return c.Next() }
tenantDB := func(c *fiber.Ctx) error { executionOrder = append(executionOrder, "tenantDB"); return c.Next() }
result := buildProtectedMiddlewares(idempotency, rateLimiter, tenantDB)
assert.Len(t, result, 3)
// Execute middlewares to verify order
app := fiber.New()
app.Get("/test", result[0], result[1], result[2], func(c *fiber.Ctx) error { return nil })
req := httptest.NewRequest(http.MethodGet, "/test", nil)
_, _ = app.Test(req)
assert.Equal(t, []string{"tenantDB", "rateLimiter", "idempotency"}, executionOrder)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/routes_test.go` around lines 738 - 749, The test
TestBuildProtectedMiddlewares_AllMiddlewares only checks the slice length but
not execution order; update the test to verify that buildProtectedMiddlewares
returns middlewares in the expected sequence by executing them and asserting the
observed execution order is [tenantDB, rateLimiter, idempotency]; instrument the
idempotency, rateLimiter, and tenantDB handlers to append identifiers to an
executionOrder slice, wire result[0], result[1], result[2] into a temporary
Fiber route, invoke it with httptest.NewRequest, and assert the executionOrder
equals the expected slice.
Comment on lines
+105
to
+113
| mockManager := &mockRabbitMQMultiTenantManager{} | ||
|
|
||
| publisher, err := NewEventPublisherMultiTenant(mockManager) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify the publisher is in multi-tenant mode | ||
| assert.NotNil(t, publisher.rabbitmqManager) | ||
| assert.Nil(t, publisher.confirmablePublisher) | ||
| } |
There was a problem hiding this comment.
Field name mismatch repeated.
Same issue: publisher.rabbitmqManager should be publisher.rmqManager.
🔧 Proposed fix
- assert.NotNil(t, publisher.rabbitmqManager)
+ assert.NotNil(t, publisher.rmqManager)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mockManager := &mockRabbitMQMultiTenantManager{} | |
| publisher, err := NewEventPublisherMultiTenant(mockManager) | |
| require.NoError(t, err) | |
| // Verify the publisher is in multi-tenant mode | |
| assert.NotNil(t, publisher.rabbitmqManager) | |
| assert.Nil(t, publisher.confirmablePublisher) | |
| } | |
| mockManager := &mockRabbitMQMultiTenantManager{} | |
| publisher, err := NewEventPublisherMultiTenant(mockManager) | |
| require.NoError(t, err) | |
| // Verify the publisher is in multi-tenant mode | |
| assert.NotNil(t, publisher.rmqManager) | |
| assert.Nil(t, publisher.confirmablePublisher) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ingestion/adapters/rabbitmq/event_publisher_test.go` around lines
105 - 113, The test uses the wrong field name: update assertions in Test (where
you construct NewEventPublisherMultiTenant with mockRabbitMQMultiTenantManager)
to reference publisher.rmqManager instead of publisher.rabbitmqManager; change
the NotNil assertion to assert.NotNil(t, publisher.rmqManager) and keep the
confirmablePublisher check as assert.Nil(t, publisher.confirmablePublisher) so
the test aligns with the EventPublisherMultiTenant struct's actual field names.
Comment on lines
+125
to
+132
| err = publisher.publishMultiTenant( | ||
| context.Background(), | ||
| "test.routing.key", | ||
| "", // empty tenant ID | ||
| amqp.Publishing{}, | ||
| nil, | ||
| nil, | ||
| ) |
There was a problem hiding this comment.
Function signature mismatch will cause compile error.
publishMultiTenant in production (line 166-171) has signature (ctx, tenantID, exchange, routingKey, msg) with 5 parameters. This test passes 6 arguments including two nil values and swaps the parameter order (routingKey before tenantID).
🔧 Proposed fix
err = publisher.publishMultiTenant(
context.Background(),
- "test.routing.key",
"", // empty tenant ID
+ sharedRabbitmq.ExchangeName,
+ "test.routing.key",
amqp.Publishing{},
- nil,
- nil,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| err = publisher.publishMultiTenant( | |
| context.Background(), | |
| "test.routing.key", | |
| "", // empty tenant ID | |
| amqp.Publishing{}, | |
| nil, | |
| nil, | |
| ) | |
| err = publisher.publishMultiTenant( | |
| context.Background(), | |
| "", // empty tenant ID | |
| sharedRabbitmq.ExchangeName, | |
| "test.routing.key", | |
| amqp.Publishing{}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ingestion/adapters/rabbitmq/event_publisher_test.go` around lines
125 - 132, The test is calling publishMultiTenant with the wrong parameter order
and extra nils; update the call to match the production signature
publishMultiTenant(ctx, tenantID, exchange, routingKey, msg) by passing
context.Background(), the tenant ID ("" for empty), an exchange (use "" if
none), the routing key "test.routing.key", and a single amqp.Publishing value
(remove the two nil arguments) so the call has exactly five arguments in the
correct order.
Comment on lines
+58
to
+65
| // getTenantPrefixedKey returns a tenant-prefixed object key using canonical lib-commons v4 | ||
| // s3.GetObjectStorageKeyForTenant. | ||
| // In multi-tenant mode (tenantID in context): "{tenantID}/{key}" | ||
| // In single-tenant mode (no tenant in context): "{key}" unchanged | ||
| // Leading slashes are always stripped from the key for clean path construction. | ||
| func getTenantPrefixedKey(ctx context.Context, key string) (string, error) { | ||
| return tms3.GetS3KeyStorageContext(ctx, key) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files internal/reporting/adapters/storage/ | head -20Repository: LerianStudio/matcher
Length of output: 132
🏁 Script executed:
# Check if the file exists and get its size
wc -l internal/reporting/adapters/storage/s3_client.go 2>/dev/null || echo "File not found"Repository: LerianStudio/matcher
Length of output: 116
🏁 Script executed:
# Read lines 50-75 to see context around the helper function
sed -n '50,75p' internal/reporting/adapters/storage/s3_client.goRepository: LerianStudio/matcher
Length of output: 1190
🏁 Script executed:
# Check the import for tms3 to confirm the package
grep -n "tms3" internal/reporting/adapters/storage/s3_client.go | head -5Repository: LerianStudio/matcher
Length of output: 188
🏁 Script executed:
# Check for logger usage and error handling in S3 methods
grep -n "logger\|fmt.Errorf\|return.*err" internal/reporting/adapters/storage/s3_client.go | head -20Repository: LerianStudio/matcher
Length of output: 1447
Wrap the external error and update the helper name in the doc comment.
Line 59 documents the old API name s3.GetObjectStorageKeyForTenant, but the code calls tms3.GetS3KeyStorageContext. Line 64 returns the external error directly without wrapping, violating the guideline to use fmt.Errorf() with %w verb for error wrapping.
Proposed fix
-// s3.GetObjectStorageKeyForTenant.
+// tms3.GetS3KeyStorageContext.
// In multi-tenant mode (tenantID in context): "{tenantID}/{key}"
// In single-tenant mode (no tenant in context): "{key}" unchanged
// Leading slashes are always stripped from the key for clean path construction.
func getTenantPrefixedKey(ctx context.Context, key string) (string, error) {
- return tms3.GetS3KeyStorageContext(ctx, key)
+ prefixedKey, err := tms3.GetS3KeyStorageContext(ctx, key)
+ if err != nil {
+ return "", fmt.Errorf("get tenant-prefixed s3 key: %w", err)
+ }
+ return prefixedKey, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // getTenantPrefixedKey returns a tenant-prefixed object key using canonical lib-commons v4 | |
| // s3.GetObjectStorageKeyForTenant. | |
| // In multi-tenant mode (tenantID in context): "{tenantID}/{key}" | |
| // In single-tenant mode (no tenant in context): "{key}" unchanged | |
| // Leading slashes are always stripped from the key for clean path construction. | |
| func getTenantPrefixedKey(ctx context.Context, key string) (string, error) { | |
| return tms3.GetS3KeyStorageContext(ctx, key) | |
| } | |
| // getTenantPrefixedKey returns a tenant-prefixed object key using canonical lib-commons v4 | |
| // tms3.GetS3KeyStorageContext. | |
| // In multi-tenant mode (tenantID in context): "{tenantID}/{key}" | |
| // In single-tenant mode (no tenant in context): "{key}" unchanged | |
| // Leading slashes are always stripped from the key for clean path construction. | |
| func getTenantPrefixedKey(ctx context.Context, key string) (string, error) { | |
| prefixedKey, err := tms3.GetS3KeyStorageContext(ctx, key) | |
| if err != nil { | |
| return "", fmt.Errorf("get tenant-prefixed s3 key: %w", err) | |
| } | |
| return prefixedKey, nil | |
| } |
🧰 Tools
🪛 GitHub Check: Go Analysis / Lint (matcher)
[failure] 64-64:
error returned from external package is unwrapped: sig: func github.com/LerianStudio/lib-commons/v4/commons/tenant-manager/s3.GetS3KeyStorageContext(ctx context.Context, key string) (string, error) (wrapcheck)
🪛 golangci-lint (2.11.4)
[error] 64-64: error returned from external package is unwrapped: sig: func github.com/LerianStudio/lib-commons/v4/commons/tenant-manager/s3.GetS3KeyStorageContext(ctx context.Context, key string) (string, error)
(wrapcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/reporting/adapters/storage/s3_client.go` around lines 58 - 65,
Update the doc comment for getTenantPrefixedKey to reference the current API
name tms3.GetS3KeyStorageContext (replace the old
s3.GetObjectStorageKeyForTenant mention) and modify the function to wrap any
error returned from tms3.GetS3KeyStorageContext using fmt.Errorf with the %w
verb before returning; keep the behavior of stripping leading slashes and the
tenant-prefix logic but ensure errors are wrapped for proper propagation.
Comment on lines
+139
to
+152
| func TestNewTenantMetricsFromConfig_MultiTenantEnabled_ReturnsRealMetrics(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // NewTenantMetricsFromConfig uses global otel.Meter which uses noop provider by default | ||
| // This test verifies the function returns valid metrics without error | ||
| metrics, err := NewTenantMetricsFromConfig(true) | ||
|
|
||
| require.NoError(t, err) | ||
| require.NotNil(t, metrics) | ||
| assert.NotNil(t, metrics.ConnectionsTotal) | ||
| assert.NotNil(t, metrics.ConnectionErrorsTotal) | ||
| assert.NotNil(t, metrics.ConsumersActive) | ||
| assert.NotNil(t, metrics.MessagesProcessed) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential test order dependency due to singleton.
This test is marked t.Parallel() but calls NewTenantMetricsFromConfig(true) which uses the singleton. If TestNewTenantMetrics_SingletonBehavior runs first, the singleton will already be initialized with a different meter. The test still passes because both use no-op providers, but this creates a hidden dependency.
Consider either:
- Remove
t.Parallel()from this test - Add a comment noting the singleton behavior doesn't affect this test because global otel.Meter defaults to no-op
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/shared/infrastructure/metrics/tenant_metrics_test.go` around lines
139 - 152, The test
TestNewTenantMetricsFromConfig_MultiTenantEnabled_ReturnsRealMetrics calls the
singleton-using NewTenantMetricsFromConfig(true) and is marked t.Parallel(),
creating a hidden order dependency with TestNewTenantMetrics_SingletonBehavior;
either remove the t.Parallel() call from
TestNewTenantMetricsFromConfig_MultiTenantEnabled_ReturnsRealMetrics to ensure
deterministic initialization of the singleton, or if you prefer to keep it, add
a clear comment above the test referencing NewTenantMetricsFromConfig and the
singleton behavior stating why parallelism is safe (global otel.Meter defaults
to noop) so future maintainers understand the dependency.
Comment on lines
+144
to
+147
| // Use the exported constant DefaultTenantSlug for comparison | ||
| // since getDefaultTenantSlug() is not exported | ||
| defaultTenantSlug := auth.DefaultTenantSlug | ||
|
|
There was a problem hiding this comment.
Fallback assertion is tied to a constant, not the runtime default slug.
Line 146 uses auth.DefaultTenantSlug, but auth.GetTenantSlug falls back to the runtime default (getDefaultTenantSlug()), which can be configured. This can create false failures when defaults are overridden in test setup.
Suggested fix
- // Use the exported constant DefaultTenantSlug for comparison
- // since getDefaultTenantSlug() is not exported
- defaultTenantSlug := auth.DefaultTenantSlug
+ // Capture the current runtime default used by GetTenantSlug fallback
+ defaultTenantSlug := auth.GetTenantSlug(nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/shared/infrastructure/tenant/adapters/multi_tenant_test.go` around
lines 144 - 147, The test currently binds defaultTenantSlug to the compile-time
constant auth.DefaultTenantSlug which can mismatch runtime defaults; instead
obtain the effective default at runtime by calling the public accessor
auth.GetTenantSlug (or the proper exported function that returns the current
tenant slug) and use that value in the assertion (replace auth.DefaultTenantSlug
/ defaultTenantSlug with the result of auth.GetTenantSlug) so the test uses the
same fallback logic as getDefaultTenantSlug().
| parentCtx := context.WithValue(context.Background(), auth.TenantIDKey, tenantID) | ||
|
|
||
| // Create child context with additional value | ||
| childCtx := context.WithValue(parentCtx, "custom_key", "custom_value") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify string literal keys passed to context.WithValue in this file
rg -nP 'context\.WithValue\([^,]+,\s*"[^"]+"' internal/shared/infrastructure/tenant/adapters/multi_tenant_test.goRepository: LerianStudio/matcher
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Check the test file structure, including build tags and surrounding context
head -20 internal/shared/infrastructure/tenant/adapters/multi_tenant_test.goRepository: LerianStudio/matcher
Length of output: 540
🏁 Script executed:
#!/bin/bash
# Search for all context.WithValue calls in the file to see if there's a pattern
rg -n 'context\.WithValue' internal/shared/infrastructure/tenant/adapters/multi_tenant_test.go -B 2 -A 2Repository: LerianStudio/matcher
Length of output: 1647
Use a dedicated key type instead of a raw string for context.WithValue.
Line 206 uses a raw string key ("custom_key") while all other calls in this file use proper key types (e.g., auth.TenantIDKey, auth.TenantSlugKey). Apply the same pattern here to prevent key collisions and maintain consistency.
Suggested fix
+ type testContextKey string
+ const customKey testContextKey = "custom_key"
- childCtx := context.WithValue(parentCtx, "custom_key", "custom_value")
+ childCtx := context.WithValue(parentCtx, customKey, "custom_value")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| childCtx := context.WithValue(parentCtx, "custom_key", "custom_value") | |
| type testContextKey string | |
| const customKey testContextKey = "custom_key" | |
| childCtx := context.WithValue(parentCtx, customKey, "custom_value") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/shared/infrastructure/tenant/adapters/multi_tenant_test.go` at line
206, Replace the raw string key used in context.WithValue when creating
childCtx: instead of context.WithValue(parentCtx, "custom_key", "custom_value")
define and use a dedicated typed key (e.g., type ctxKey string and a const like
customKey ctxKey = "custom_key") or reuse an existing auth.*Key type if
appropriate, then call context.WithValue(parentCtx, customKey, "custom_value");
update references to the key accordingly to match the pattern used by
auth.TenantIDKey/auth.TenantSlugKey and avoid collisions.
…enant discovery - Upgrade lib-commons from v4.5.0 to v4.6.0 - Add MULTI_TENANT_REDIS_TLS env var (14th canonical env var) - Add MULTI_TENANT_REDIS_HOST/PORT/PASSWORD to config and systemplane keys - Wire event-driven tenant discovery: TenantCache + PubSub Redis client + TenantEventListener + EventDispatcher for tenant lifecycle events - Pass WithTenantCache to TenantMiddleware when cache is available - Fix S3 comments referencing old GetObjectStorageKeyForTenant name - Adapt tests for v4.6.0 API changes (ContextWithTenantID)
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
Contributor
|
This PR is very large (36 files, 2084 lines changed). Consider breaking it into smaller PRs for easier review. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/systemplane_keys_tenancy_test.go (1)
104-127: 🧹 Nitpick | 🔵 TrivialConsider adding a test to verify the Redis password key is marked as secret.
The test correctly validates the new tenancy Redis configuration keys. However, following the pattern established by
TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret(lines 195-211), consider adding a similar test to verify thattenancy.multi_tenant_redis_passwordis correctly marked asSecret: truewithRedactFullpolicy.♻️ Proposed test addition
func TestMatcherKeyDefsTenancyConnectivity_RedisPasswordIsSecret(t *testing.T) { t.Parallel() defs := matcherKeyDefsTenancyConnectivity() var found bool for _, def := range defs { if def.Key == "tenancy.multi_tenant_redis_password" { found = true assert.True(t, def.Secret, "redis password must be marked as secret") assert.Equal(t, domain.RedactFull, def.RedactPolicy, "redis password must use full redaction") } } assert.True(t, found, "tenancy.multi_tenant_redis_password must exist") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/systemplane_keys_tenancy_test.go` around lines 104 - 127, Add a unit test that asserts the tenancy Redis password key is marked secret and uses full redaction: in the test package add a test (similar to TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret) that calls matcherKeyDefsTenancyConnectivity(), iterates defs to find the def with Key == "tenancy.multi_tenant_redis_password", and asserts def.Secret is true and def.RedactPolicy equals domain.RedactFull, and finally assert the key was found.
♻️ Duplicate comments (3)
internal/bootstrap/routes_test.go (1)
705-708:⚠️ Potential issue | 🟠 MajorReplace removed middleware tests with
RegisterRoutesorder verification, not a note.Line 705–708 documents test removal, but this drops behavioral coverage for middleware sequencing. Please add a replacement test that executes a protected route and asserts effective order (tenant middleware before rate-limit/idempotency path), since ordering is now assembled inline in
RegisterRoutes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/routes_test.go` around lines 705 - 708, Add a replacement test that verifies the middleware ordering assembled inline in RegisterRoutes: create a test (e.g. TestRegisterRoutes_ProtectedMiddlewareOrder) that registers the routes via RegisterRoutes, mounts a protected endpoint, then invokes that endpoint and asserts the execution sequence so tenant middleware runs before rate-limit and idempotency middleware; use the same middleware identifiers used in production (tenant middleware, rate-limit middleware, idempotency middleware) to detect invocation order and fail if the order is incorrect, mirroring the former TestBuildProtectedMiddlewares_* behavioral checks.docs/multi-tenant-guide.md (1)
18-34:⚠️ Potential issue | 🟠 MajorAdd
AUTH_ENABLED=trueto the activation prerequisites.The current env table and activation snippet can produce an invalid startup config in multi-tenant mode because auth is now mandatory when
MULTI_TENANT_ENABLED=true.🛠 Suggested patch
| `MULTI_TENANT_ENABLED` | bool | `false` | No | Master switch for multi-tenant mode | +| `AUTH_ENABLED` | bool | `false` | When multi-tenant is enabled | Must be `true` when `MULTI_TENANT_ENABLED=true` | | `MULTI_TENANT_URL` | string | — | When enabled | Tenant Manager API URL | @@ export MULTI_TENANT_ENABLED=true +export AUTH_ENABLED=true export MULTI_TENANT_URL=http://tenant-manager:8080 export MULTI_TENANT_ENVIRONMENT=production🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/multi-tenant-guide.md` around lines 18 - 34, Add the required AUTH_ENABLED setting to the multi-tenant docs: update the environment variables table to include `AUTH_ENABLED` (type: bool, default: `false`, Required: Yes when `MULTI_TENANT_ENABLED` is true) and update the "How to Activate" snippet to export `AUTH_ENABLED=true` alongside `MULTI_TENANT_ENABLED`, `MULTI_TENANT_URL`, and `MULTI_TENANT_ENVIRONMENT` so startup config is valid when enabling multi-tenant mode.internal/reporting/adapters/storage/s3_client.go (1)
58-65:⚠️ Potential issue | 🟡 MinorWrap the error returned from external package.
The error from
tms3.GetS3KeyStorageContextis returned directly without wrapping, violating the coding guideline to usefmt.Errorf()with%wverb for error wrapping. This was also flagged by static analysis (wrapcheck).🔧 Proposed fix
func getTenantPrefixedKey(ctx context.Context, key string) (string, error) { - return tms3.GetS3KeyStorageContext(ctx, key) + prefixedKey, err := tms3.GetS3KeyStorageContext(ctx, key) + if err != nil { + return "", fmt.Errorf("get tenant-prefixed s3 key: %w", err) + } + return prefixedKey, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/reporting/adapters/storage/s3_client.go` around lines 58 - 65, The function getTenantPrefixedKey returns the error from tms3.GetS3KeyStorageContext directly; update it to wrap the returned error using fmt.Errorf with the %w verb (e.g., call tms3.GetS3KeyStorageContext(ctx, key), and if err != nil return "", fmt.Errorf("get tenant prefixed key: %w", err)) so the original error is preserved and static analysis (wrapcheck) is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/multi-tenant-guide.md`:
- Around line 9-15: Replace outdated API/version references: update every
occurrence of valkey.GetKeyFromContext to valkey.GetKeyContext (e.g., in the
Ingestion and Reporting rows where Redis key prefixing is described) and change
lib-commons v3 to lib-commons v4 (e.g., in the Bootstrap row and the
footer/summary where the library version is mentioned); search the document for
these exact symbols to find and replace all instances so the guide matches the
current codebase.
In `@internal/bootstrap/config.go`:
- Around line 70-74: Document the optional nature of MULTI_TENANT_REDIS_HOST and
the fallback behavior when omitted by updating the MultiTenantRedisHost field
comment in internal/bootstrap/config.go to state that an empty value disables
event-driven tenant discovery (see tenant_event_discovery.go), and either add a
warning-level log in the tenant discovery startup path when multi-tenant mode is
enabled but MultiTenantRedisHost is empty or add a clear check in
validateTenancyConfig that emits a warning (or error if desired) so operators
aren’t confused; reference the MultiTenantRedisHost field, validateTenancyConfig
function, and tenant_event_discovery.go startup logic when making the change.
In `@internal/bootstrap/init.go`:
- Around line 1443-1452: The first return value from initTenantEventDiscovery
(the TenantEventListener) is being discarded; either preserve it on the provider
for future use (metrics/status/testing) or add a clarifying comment that it is
intentionally unused because eventCleanup handles lifecycle. Update the code
that calls initTenantEventDiscovery to capture the listener (e.g., listener,
tenantCache, eventCleanup := initTenantEventDiscovery(...)) and then store the
listener on the provider (e.g., provider.tenantEventListener = listener) guarded
by provider.mu, or alternatively add a brief comment next to the call
referencing TenantEventListener and explaining why the value is intentionally
discarded.
In `@internal/bootstrap/systemplane_keys_tenancy.go`:
- Around line 118-170: The four tenancy Redis Pub/Sub config keys
(tenancy.multi_tenant_redis_host, tenancy.multi_tenant_redis_port,
tenancy.multi_tenant_redis_password, tenancy.multi_tenant_redis_tls) are marked
ApplyBundleRebuild and MutableAtRuntime but initTenantEventDiscovery is only run
at startup, so runtime bundle rebuilds won't reconnect; fix by either adding a
reconciler or marking the keys bootstrap-only: implement a
TenantEventDiscoveryReconciler (or extend the existing reconciler set used in
init.go where HTTP/Publisher/Worker reconcilers are registered) that calls
initTenantEventDiscovery on bundle rebuilds, or change those keys' ApplyBehavior
to ApplyBootstrapOnly and update docs to state restart is required for changes.
In `@internal/bootstrap/tenant_event_discovery_test.go`:
- Around line 92-121: Add an integration test that covers the successful
initialization path of initTenantEventDiscovery when multi-tenant mode and a
Redis host are provided: create a new test file using the integration build tag
(//go:build integration) and a test like TestInitTenantEventDiscovery_Enabled
that boots a Redis testcontainer, sets cfg.Tenancy.MultiTenantEnabled = true and
cfg.Tenancy.MultiTenantRedisHost to the container address, calls
initTenantEventDiscovery(cfg, nil, nil), asserts listener and cache are non-nil
and cleanup is non-nil, exercises cleanup (which should stop/close the discovery
and Redis client) and ensures the testcontainer is terminated in teardown;
reference initTenantEventDiscovery and the existing test names
(TestInitTenantEventDiscovery_DisabledWhenSingleTenant,
TestInitTenantEventDiscovery_DisabledWhenRedisHostEmpty) to mirror structure and
assertions.
In `@internal/bootstrap/tenant_event_discovery.go`:
- Around line 7-19: Reorder and format the import block in
tenant_event_discovery.go to satisfy gci: place standard library imports first
(context, fmt), then third-party packages (github.com/redis/go-redis/v9), then
Lerian packages (github.com/LerianStudio/... entries like lib-commons imports:
libLog, tmevent, tmpostgres, tmredis, tenantcache), and finally project-local
imports (github.com/LerianStudio/matcher/internal/shared/constants); ensure
grouping and blank lines between groups and that alias names (libLog, tmevent,
tmpostgres, tmredis) remain unchanged so functions/types that reference them
still compile.
- Around line 42-138: The function initTenantEventDiscovery exceeds allowed
cyclomatic complexity; extract the Redis Pub/Sub client and TenantEventListener
creation and startup into a helper (e.g., createTenantPubSubListener or
setupTenantPubSub) so initTenantEventDiscovery only performs configuration
checks, constructs tenantcache.NewTenantCache, calls the helper, and wires the
returned listener/cache/cleanup; the helper should encapsulate
tmredis.NewTenantPubSubRedisClient, tmevent.NewEventDispatcher,
tmevent.NewTenantEventListener, listener.Start, and any immediate error
handling/closeRedisClient calls and return (pubsubClient, listener, cleanupFunc,
err) or equivalent to keep branching out of initTenantEventDiscovery and reduce
its cyclomatic complexity.
---
Outside diff comments:
In `@internal/bootstrap/systemplane_keys_tenancy_test.go`:
- Around line 104-127: Add a unit test that asserts the tenancy Redis password
key is marked secret and uses full redaction: in the test package add a test
(similar to TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret) that
calls matcherKeyDefsTenancyConnectivity(), iterates defs to find the def with
Key == "tenancy.multi_tenant_redis_password", and asserts def.Secret is true and
def.RedactPolicy equals domain.RedactFull, and finally assert the key was found.
---
Duplicate comments:
In `@docs/multi-tenant-guide.md`:
- Around line 18-34: Add the required AUTH_ENABLED setting to the multi-tenant
docs: update the environment variables table to include `AUTH_ENABLED` (type:
bool, default: `false`, Required: Yes when `MULTI_TENANT_ENABLED` is true) and
update the "How to Activate" snippet to export `AUTH_ENABLED=true` alongside
`MULTI_TENANT_ENABLED`, `MULTI_TENANT_URL`, and `MULTI_TENANT_ENVIRONMENT` so
startup config is valid when enabling multi-tenant mode.
In `@internal/bootstrap/routes_test.go`:
- Around line 705-708: Add a replacement test that verifies the middleware
ordering assembled inline in RegisterRoutes: create a test (e.g.
TestRegisterRoutes_ProtectedMiddlewareOrder) that registers the routes via
RegisterRoutes, mounts a protected endpoint, then invokes that endpoint and
asserts the execution sequence so tenant middleware runs before rate-limit and
idempotency middleware; use the same middleware identifiers used in production
(tenant middleware, rate-limit middleware, idempotency middleware) to detect
invocation order and fail if the order is incorrect, mirroring the former
TestBuildProtectedMiddlewares_* behavioral checks.
In `@internal/reporting/adapters/storage/s3_client.go`:
- Around line 58-65: The function getTenantPrefixedKey returns the error from
tms3.GetS3KeyStorageContext directly; update it to wrap the returned error using
fmt.Errorf with the %w verb (e.g., call tms3.GetS3KeyStorageContext(ctx, key),
and if err != nil return "", fmt.Errorf("get tenant prefixed key: %w", err)) so
the original error is preserved and static analysis (wrapcheck) is satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ac74276-399e-4205-90db-1ab13825d59b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
docs/multi-tenant-guide.mdgo.modinternal/bootstrap/config.gointernal/bootstrap/config_manager_systemplane.gointernal/bootstrap/config_override_env_keys_test.gointernal/bootstrap/config_tenancy_test.gointernal/bootstrap/config_test_helpers_test.gointernal/bootstrap/dynamic_infrastructure_provider.gointernal/bootstrap/init.gointernal/bootstrap/multi_tenant_isolation_test.gointernal/bootstrap/multi_tenant_unit_test.gointernal/bootstrap/routes_test.gointernal/bootstrap/systemplane_keys.gointernal/bootstrap/systemplane_keys_tenancy.gointernal/bootstrap/systemplane_keys_tenancy_test.gointernal/bootstrap/systemplane_keys_test.gointernal/bootstrap/tenant_event_discovery.gointernal/bootstrap/tenant_event_discovery_test.gointernal/reporting/adapters/storage/s3_client.gointernal/reporting/adapters/storage/s3_client_coverage_test.go
Comment on lines
+9
to
+15
| | Ingestion | `matcher` | `ingestion` | Redis dedupe, RabbitMQ publisher | Redis key prefixing via `valkey.GetKeyFromContext`, RabbitMQ per-tenant vhosts via `tmrabbitmq.Manager`, X-Tenant-ID headers | | ||
| | Matching | `matcher` | `matching` | RabbitMQ publisher | RabbitMQ per-tenant vhosts via `tmrabbitmq.Manager`, X-Tenant-ID headers | | ||
| | Reporting | `matcher` | `reporting` | Redis cache, S3 storage | Redis key prefixing via `valkey.GetKeyFromContext`, S3 key prefixing via `tms3.GetS3KeyStorageContext` | | ||
| | Configuration | `matcher` | `configuration` | PostgreSQL (via middleware) | Tenant middleware resolves DB connections | | ||
| | Governance | `matcher` | `governance` | PostgreSQL (via middleware) | Tenant middleware resolves DB connections | | ||
| | Bootstrap | `matcher` | N/A | TenantMiddleware, PostgresManager, RabbitMQManager | lib-commons v3 tenant-manager components | | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify documentation references against repository usage.
rg -n "GetKeyFromContext|GetKeyContext|lib-commons v3|lib-commons v4" docs/multi-tenant-guide.md
rg -n "GetKeyContext\\(" --type=go
rg -n "github.com/LerianStudio/lib-commons/v4" go.modRepository: LerianStudio/matcher
Length of output: 2225
Update outdated API/version references in the guide.
The doc cites valkey.GetKeyFromContext and lib-commons v3, while the codebase uses valkey.GetKeyContext and lib-commons/v4. Update lines 9, 11, 14, and 78:
- Change
valkey.GetKeyFromContext→valkey.GetKeyContext(lines 9, 11) - Change
lib-commons v3→lib-commons v4(lines 14, 78)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/multi-tenant-guide.md` around lines 9 - 15, Replace outdated API/version
references: update every occurrence of valkey.GetKeyFromContext to
valkey.GetKeyContext (e.g., in the Ingestion and Reporting rows where Redis key
prefixing is described) and change lib-commons v3 to lib-commons v4 (e.g., in
the Bootstrap row and the footer/summary where the library version is
mentioned); search the document for these exact symbols to find and replace all
instances so the guide matches the current codebase.
Comment on lines
+70
to
74
| MultiTenantRedisHost string `env:"MULTI_TENANT_REDIS_HOST" mapstructure:"multi_tenant_redis_host"` | ||
| MultiTenantRedisPort string `env:"MULTI_TENANT_REDIS_PORT" envDefault:"6379" mapstructure:"multi_tenant_redis_port"` | ||
| MultiTenantRedisPassword string `env:"MULTI_TENANT_REDIS_PASSWORD" json:"-" mapstructure:"multi_tenant_redis_password"` | ||
| MultiTenantRedisTLS bool `env:"MULTI_TENANT_REDIS_TLS" envDefault:"false" mapstructure:"multi_tenant_redis_tls"` | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting that MULTI_TENANT_REDIS_HOST is optional for event-driven tenant discovery.
The new multi-tenant Redis configuration fields are correctly defined. However, based on the context from tenant_event_discovery.go, when MultiTenantRedisHost is empty, event-driven tenant discovery is silently disabled (only an info-level log). This behavior is not validated by validateTenancyConfig, which could lead to operator confusion if they expect event discovery to be active.
Consider one of:
- Adding a comment to the field documenting that it's optional and what happens when omitted
- Adding a warning-level log when multi-tenant is enabled but Redis host is not configured
- Making it a validation error if operators explicitly expect event discovery
📝 Proposed documentation addition
- MultiTenantRedisHost string `env:"MULTI_TENANT_REDIS_HOST" mapstructure:"multi_tenant_redis_host"`
+ // MultiTenantRedisHost is the Redis host for tenant event discovery via Pub/Sub.
+ // When empty, event-driven tenant discovery is disabled and tenant resolution
+ // falls back to direct tenant-manager API calls (higher latency, no caching).
+ MultiTenantRedisHost string `env:"MULTI_TENANT_REDIS_HOST" mapstructure:"multi_tenant_redis_host"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| MultiTenantRedisHost string `env:"MULTI_TENANT_REDIS_HOST" mapstructure:"multi_tenant_redis_host"` | |
| MultiTenantRedisPort string `env:"MULTI_TENANT_REDIS_PORT" envDefault:"6379" mapstructure:"multi_tenant_redis_port"` | |
| MultiTenantRedisPassword string `env:"MULTI_TENANT_REDIS_PASSWORD" json:"-" mapstructure:"multi_tenant_redis_password"` | |
| MultiTenantRedisTLS bool `env:"MULTI_TENANT_REDIS_TLS" envDefault:"false" mapstructure:"multi_tenant_redis_tls"` | |
| } | |
| // MultiTenantRedisHost is the Redis host for tenant event discovery via Pub/Sub. | |
| // When empty, event-driven tenant discovery is disabled and tenant resolution | |
| // falls back to direct tenant-manager API calls (higher latency, no caching). | |
| MultiTenantRedisHost string `env:"MULTI_TENANT_REDIS_HOST" mapstructure:"multi_tenant_redis_host"` | |
| MultiTenantRedisPort string `env:"MULTI_TENANT_REDIS_PORT" envDefault:"6379" mapstructure:"multi_tenant_redis_port"` | |
| MultiTenantRedisPassword string `env:"MULTI_TENANT_REDIS_PASSWORD" json:"-" mapstructure:"multi_tenant_redis_password"` | |
| MultiTenantRedisTLS bool `env:"MULTI_TENANT_REDIS_TLS" envDefault:"false" mapstructure:"multi_tenant_redis_tls"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/config.go` around lines 70 - 74, Document the optional
nature of MULTI_TENANT_REDIS_HOST and the fallback behavior when omitted by
updating the MultiTenantRedisHost field comment in internal/bootstrap/config.go
to state that an empty value disables event-driven tenant discovery (see
tenant_event_discovery.go), and either add a warning-level log in the tenant
discovery startup path when multi-tenant mode is enabled but
MultiTenantRedisHost is empty or add a clear check in validateTenancyConfig that
emits a warning (or error if desired) so operators aren’t confused; reference
the MultiTenantRedisHost field, validateTenancyConfig function, and
tenant_event_discovery.go startup logic when making the change.
Comment on lines
+1443
to
+1452
| // Wire event-driven tenant discovery (TenantCache + Pub/Sub listener). | ||
| // When MULTI_TENANT_REDIS_HOST is configured, tenant lifecycle events from | ||
| // tenant-manager are received via Redis Pub/Sub and update the shared cache. | ||
| // The cache is passed to the TenantMiddleware for cache-first resolution. | ||
| _, tenantCache, eventCleanup := initTenantEventDiscovery(cfg, pgManager, cfg.Logger) | ||
|
|
||
| provider.mu.Lock() | ||
| provider.tenantCache = tenantCache | ||
| provider.eventDiscoveryCleanup = eventCleanup | ||
| provider.mu.Unlock() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify why the TenantEventListener return value is discarded.
The first return value from initTenantEventDiscovery (the TenantEventListener) is discarded. If the listener is only needed internally for cleanup purposes (handled by eventCleanup), this is fine. However, if there are scenarios where the listener reference would be useful (e.g., for metrics, status inspection, or testing), consider storing it on the provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/init.go` around lines 1443 - 1452, The first return value
from initTenantEventDiscovery (the TenantEventListener) is being discarded;
either preserve it on the provider for future use (metrics/status/testing) or
add a clarifying comment that it is intentionally unused because eventCleanup
handles lifecycle. Update the code that calls initTenantEventDiscovery to
capture the listener (e.g., listener, tenantCache, eventCleanup :=
initTenantEventDiscovery(...)) and then store the listener on the provider
(e.g., provider.tenantEventListener = listener) guarded by provider.mu, or
alternatively add a brief comment next to the call referencing
TenantEventListener and explaining why the value is intentionally discarded.
Comment on lines
+118
to
+170
| { | ||
| Key: "tenancy.multi_tenant_redis_host", | ||
| Kind: domain.KindConfig, | ||
| AllowedScopes: []domain.Scope{domain.ScopeGlobal}, | ||
| DefaultValue: "", | ||
| ValueType: domain.ValueTypeString, | ||
| ApplyBehavior: domain.ApplyBundleRebuild, | ||
| MutableAtRuntime: true, | ||
| Description: "Redis host for tenant event Pub/Sub (event-driven discovery)", | ||
| Group: "tenancy", | ||
| Component: domain.ComponentNone, | ||
| RedactPolicy: domain.RedactNone, | ||
| }, | ||
| { | ||
| Key: "tenancy.multi_tenant_redis_port", | ||
| Kind: domain.KindConfig, | ||
| AllowedScopes: []domain.Scope{domain.ScopeGlobal}, | ||
| DefaultValue: defaultMultiTenantRedisPort, | ||
| ValueType: domain.ValueTypeString, | ||
| ApplyBehavior: domain.ApplyBundleRebuild, | ||
| MutableAtRuntime: true, | ||
| Description: "Redis port for tenant event Pub/Sub", | ||
| Group: "tenancy", | ||
| Component: domain.ComponentNone, | ||
| RedactPolicy: domain.RedactNone, | ||
| }, | ||
| { | ||
| Key: "tenancy.multi_tenant_redis_password", | ||
| Kind: domain.KindConfig, | ||
| AllowedScopes: []domain.Scope{domain.ScopeGlobal}, | ||
| DefaultValue: "", | ||
| ValueType: domain.ValueTypeString, | ||
| ApplyBehavior: domain.ApplyBundleRebuild, | ||
| MutableAtRuntime: true, | ||
| Secret: true, | ||
| Description: "Redis password for tenant event Pub/Sub", | ||
| Group: "tenancy", | ||
| Component: domain.ComponentNone, | ||
| RedactPolicy: domain.RedactFull, | ||
| }, | ||
| { | ||
| Key: "tenancy.multi_tenant_redis_tls", | ||
| Kind: domain.KindConfig, | ||
| AllowedScopes: []domain.Scope{domain.ScopeGlobal}, | ||
| DefaultValue: false, | ||
| ValueType: domain.ValueTypeBool, | ||
| ApplyBehavior: domain.ApplyBundleRebuild, | ||
| MutableAtRuntime: true, | ||
| Description: "Enable TLS for tenant event Pub/Sub Redis connection", | ||
| Group: "tenancy", | ||
| Component: domain.ComponentNone, | ||
| RedactPolicy: domain.RedactNone, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if bundle rebuild logic triggers re-initialization of tenant event discovery
rg -n -C5 "ApplyBundleRebuild" --type=go | head -100
# Search for bundle rebuild handlers that might reinitialize event discovery
rg -n "eventDiscoveryCleanup|initTenantEventDiscovery" --type=goRepository: LerianStudio/matcher
Length of output: 9882
🏁 Script executed:
# Search for bundle rebuild event handling and reconciliation logic
rg -n "ApplyBundleRebuild|reconcile" --type=go -A3 -B3 | grep -E "(func|handle|reconcile|rebuild)" | head -50
# Check the exact context where eventCleanup is invoked
rg -n -B10 -A10 "eventDiscoveryCleanup\(\)" --type=go
# Look for any configuration change listeners or hooks
rg -n "configManager|Get\(" --type=go | grep -i "tenancy\|redis" | head -20Repository: LerianStudio/matcher
Length of output: 8517
🏁 Script executed:
# Search for all reconciler implementations
fd -t f "reconciler" --type=go | xargs rg -l "Reconciler" | head -20
# Check the structure of existing reconcilers to understand the pattern
rg -n "type.*Reconciler" --type=go
# Specifically look for tenant event discovery or multi-tenant reconciler
rg -n "TenantEventDiscovery|EventDiscovery.*Reconcil" --type=go
# Check what reconcilers are actually built and used
rg -n "NewHTTPPolicyReconciler|NewPublisherReconciler|NewWorkerReconciler" --type=go -A5Repository: LerianStudio/matcher
Length of output: 19207
🏁 Script executed:
# Check if there's a tenant event discovery reconciler
rg -n "eventDiscovery|TenantEventDiscovery" --type=go | grep -i "reconcil"
# Check the PublisherReconciler to see if it handles tenant event discovery
cat -n internal/bootstrap/systemplane_reconciler_publishers.go | head -100
# Check if there's any documentation about runtime config limitations
rg -n "ApplyBundleRebuild|MutableAtRuntime" internal/bootstrap/tenant_event_discovery.go
# Verify what happens during bundle rebuild - check the actual initTenantEventDiscovery function
sed -n '32,150p' internal/bootstrap/tenant_event_discovery.goRepository: LerianStudio/matcher
Length of output: 8060
🏁 Script executed:
# Check if there's a tenant event discovery reconciler or if it's handled elsewhere
rg -n "TenantEventDiscovery|eventDiscovery" internal/bootstrap/*.go | grep -i "reconcil\|rebuild"
# Search for any reconciler that handles multi-tenant or tenancy config changes
rg -n "multiTenant\|tenancy" internal/bootstrap/systemplane_reconciler*.go
# Check if dynamic_infrastructure_provider has any bundle rebuild handling
cat -n internal/bootstrap/dynamic_infrastructure_provider.go | head -150
# Look for any listener restart or reinitialization logic in the codebase
rg -n "listener.*Start\|listener.*Stop\|eventDiscoveryCleanup" internal/bootstrap/*.go -B2 -A2Repository: LerianStudio/matcher
Length of output: 6788
🏁 Script executed:
# Check if there are any other reconcilers besides the three we found
find internal/bootstrap -name "*reconciler*.go" -type f
# Look at all reconcilers to see if any handle tenant event discovery
rg -n "Reconcile.*func" internal/bootstrap/systemplane_reconciler*.go
# Check what happens when tenancy config changes - does anything re-initialize event discovery?
rg -n "initTenantEventDiscovery" internal/bootstrap/*.go
# Look for any comment in the tenancy keys test about ApplyBundleRebuild behavior
cat -n internal/bootstrap/systemplane_keys_tenancy_test.go | sed -n '110,200p'Repository: LerianStudio/matcher
Length of output: 4314
🏁 Script executed:
# Check if there's a mechanism to reinitialize tenant event discovery during bundle rebuild
rg -n "bundle.*rebuild\|rebuild.*function" internal/bootstrap/systemplane_init.go -B3 -A3
# Look for where eventCleanup might be called during bundle rebuild
rg -n "eventDiscoveryCleanup\|Close.*provider\|rebuild" internal/bootstrap/init.go -B2 -A2
# Check if there's any loop or hook that reinitializes components after bundle rebuild
rg -n "applyChangeFeedSignal|applyBundle|reconciliers\[" internal/bootstrap/systemplane_init.go -B3 -A10 | head -60
# Verify: are there any comments or TODOs about tenant event discovery runtime changes?
rg -n "tenant.*event.*discovery\|event.*discovery.*tenant" internal/bootstrap/*.go -B2 -A2 | head -50Repository: LerianStudio/matcher
Length of output: 924
🏁 Script executed:
# Final confirmation: check if there's any code path that re-invokes initTenantEventDiscovery
rg -n "initTenantEventDiscovery" internal/bootstrap --type=go
# Verify the bundle rebuild reconciler flow - what gets called after config changes
cat -n internal/bootstrap/systemplane_init.go | sed -n '575,610p'
# Check if there are any TODOs or FIXMEs related to tenant event discovery runtime changes
rg -n "TODO|FIXME|XXX" internal/bootstrap/tenant_event_discovery.go
# Double-check: Does PublisherReconciler or any other reconciler handle tenant event discovery?
rg -n "listener\|eventDiscovery" internal/bootstrap/systemplane_reconciler*.goRepository: LerianStudio/matcher
Length of output: 2229
Add a reconciler for tenant event discovery or restrict Redis Pub/Sub keys to bootstrap-only.
The four Redis Pub/Sub configuration keys (tenancy.multi_tenant_redis_host, tenancy.multi_tenant_redis_port, tenancy.multi_tenant_redis_password, tenancy.multi_tenant_redis_tls) are marked with ApplyBundleRebuild and MutableAtRuntime: true, but initTenantEventDiscovery is invoked only once at startup (internal/bootstrap/init.go:1447) and is never re-invoked during bundle rebuild. The three built reconcilers (HTTP, Publisher, Worker) do not include one for tenant event discovery, so runtime changes to these Redis settings will not reconnect the listener to a new Redis instance.
Either add a TenantEventDiscoveryReconciler to handle reinitialization during bundle rebuild, or change these keys to ApplyBootstrapOnly and document that they require application restart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/systemplane_keys_tenancy.go` around lines 118 - 170, The
four tenancy Redis Pub/Sub config keys (tenancy.multi_tenant_redis_host,
tenancy.multi_tenant_redis_port, tenancy.multi_tenant_redis_password,
tenancy.multi_tenant_redis_tls) are marked ApplyBundleRebuild and
MutableAtRuntime but initTenantEventDiscovery is only run at startup, so runtime
bundle rebuilds won't reconnect; fix by either adding a reconciler or marking
the keys bootstrap-only: implement a TenantEventDiscoveryReconciler (or extend
the existing reconciler set used in init.go where HTTP/Publisher/Worker
reconcilers are registered) that calls initTenantEventDiscovery on bundle
rebuilds, or change those keys' ApplyBehavior to ApplyBootstrapOnly and update
docs to state restart is required for changes.
Comment on lines
+92
to
+121
| func TestInitTenantEventDiscovery_DisabledWhenSingleTenant(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cfg := defaultConfig() | ||
| cfg.Tenancy.MultiTenantEnabled = false | ||
|
|
||
| listener, cache, cleanup := initTenantEventDiscovery(cfg, nil, nil) | ||
|
|
||
| assert.Nil(t, listener, "listener must be nil in single-tenant mode") | ||
| assert.Nil(t, cache, "cache must be nil in single-tenant mode") | ||
| assert.NotNil(t, cleanup, "cleanup must always return a non-nil func") | ||
|
|
||
| // Cleanup should be a no-op and not panic. | ||
| cleanup() | ||
| } | ||
|
|
||
| func TestInitTenantEventDiscovery_DisabledWhenRedisHostEmpty(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cfg := defaultConfig() | ||
| cfg.Tenancy.MultiTenantEnabled = true | ||
| cfg.Tenancy.MultiTenantRedisHost = "" // no Redis host configured | ||
|
|
||
| listener, cache, cleanup := initTenantEventDiscovery(cfg, nil, nil) | ||
|
|
||
| assert.Nil(t, listener, "listener must be nil when Redis host is empty") | ||
| assert.Nil(t, cache, "cache must be nil when Redis host is empty") | ||
| assert.NotNil(t, cleanup, "cleanup must always return a non-nil func") | ||
| cleanup() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding integration test for the enabled scenario.
The unit tests cover disabled scenarios well, but there's no coverage for the successful initialization path where multi-tenant mode is enabled with a valid Redis host. Consider adding an integration test (with //go:build integration) that validates the full initialization flow using testcontainers for Redis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/tenant_event_discovery_test.go` around lines 92 - 121, Add
an integration test that covers the successful initialization path of
initTenantEventDiscovery when multi-tenant mode and a Redis host are provided:
create a new test file using the integration build tag (//go:build integration)
and a test like TestInitTenantEventDiscovery_Enabled that boots a Redis
testcontainer, sets cfg.Tenancy.MultiTenantEnabled = true and
cfg.Tenancy.MultiTenantRedisHost to the container address, calls
initTenantEventDiscovery(cfg, nil, nil), asserts listener and cache are non-nil
and cleanup is non-nil, exercises cleanup (which should stop/close the discovery
and Redis client) and ensures the testcontainer is terminated in teardown;
reference initTenantEventDiscovery and the existing test names
(TestInitTenantEventDiscovery_DisabledWhenSingleTenant,
TestInitTenantEventDiscovery_DisabledWhenRedisHostEmpty) to mirror structure and
assertions.
- Fix core.SetTenantIDInContext → core.ContextWithTenantID in 5 test files - Add errNilTenantChannel sentinel error for nil channel guards (err113) - Extract createAndStartListener helper to reduce cyclomatic complexity (cyclop) - Extract Redis key defs into matcherKeyDefsTenancyConnectivityRedis (funlen) - Fix import order in tenant_event_discovery.go (gci) - Wrap tms3.GetS3KeyStorageContext error return (wrapcheck) - Rewrite RabbitMQ publisher tests for v4.6.0 API compatibility
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
Contributor
|
This PR is very large (39 files, 2011 lines changed). Consider breaking it into smaller PRs for easier review. |
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 78.9% ✅ PASS |
| Threshold | 70% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/matcher/cmd/health-probe |
50.0% |
github.com/LerianStudio/matcher/cmd/matcher |
47.0% |
github.com/LerianStudio/matcher/internal/auth |
83.2% |
github.com/LerianStudio/matcher/internal/bootstrap |
71.8% |
github.com/LerianStudio/matcher/internal/configuration/adapters/audit |
94.8% |
github.com/LerianStudio/matcher/internal/configuration/adapters/http/dto |
96.2% |
github.com/LerianStudio/matcher/internal/configuration/adapters/http |
87.8% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/common |
57.1% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/context |
96.4% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/fee_rule |
98.6% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/field_map |
84.4% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/match_rule |
90.9% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/schedule |
77.7% |
github.com/LerianStudio/matcher/internal/configuration/adapters/postgres/source |
94.6% |
github.com/LerianStudio/matcher/internal/configuration/domain/entities |
91.1% |
github.com/LerianStudio/matcher/internal/configuration/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/configuration/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/configuration/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/configuration/services/command |
90.7% |
github.com/LerianStudio/matcher/internal/configuration/services/query |
100.0% |
github.com/LerianStudio/matcher/internal/configuration/services/worker |
92.7% |
github.com/LerianStudio/matcher/internal/discovery/adapters/fetcher |
86.8% |
github.com/LerianStudio/matcher/internal/discovery/adapters/http/dto |
67.8% |
github.com/LerianStudio/matcher/internal/discovery/adapters/http |
86.3% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/connection |
89.3% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/extraction |
86.3% |
github.com/LerianStudio/matcher/internal/discovery/adapters/postgres/schema |
87.8% |
github.com/LerianStudio/matcher/internal/discovery/adapters/redis |
85.5% |
github.com/LerianStudio/matcher/internal/discovery/domain/entities |
86.8% |
github.com/LerianStudio/matcher/internal/discovery/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/discovery/services/command |
65.2% |
github.com/LerianStudio/matcher/internal/discovery/services/query |
92.1% |
github.com/LerianStudio/matcher/internal/discovery/services/syncer |
78.3% |
github.com/LerianStudio/matcher/internal/discovery/services/worker |
68.1% |
github.com/LerianStudio/matcher/internal/exception/adapters/audit |
84.5% |
github.com/LerianStudio/matcher/internal/exception/adapters/http/connectors |
82.7% |
github.com/LerianStudio/matcher/internal/exception/adapters/http/dto |
100.0% |
github.com/LerianStudio/matcher/internal/exception/adapters/http |
90.4% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/comment |
81.8% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/dispute |
94.0% |
github.com/LerianStudio/matcher/internal/exception/adapters/postgres/exception |
93.2% |
github.com/LerianStudio/matcher/internal/exception/adapters/redis |
66.9% |
github.com/LerianStudio/matcher/internal/exception/adapters/resolution |
91.7% |
github.com/LerianStudio/matcher/internal/exception/adapters |
100.0% |
github.com/LerianStudio/matcher/internal/exception/domain/dispute |
99.1% |
github.com/LerianStudio/matcher/internal/exception/domain/entities |
100.0% |
github.com/LerianStudio/matcher/internal/exception/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/exception/domain/services |
89.5% |
github.com/LerianStudio/matcher/internal/exception/domain/value_objects |
99.2% |
github.com/LerianStudio/matcher/internal/exception/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/exception/ports |
50.0% |
github.com/LerianStudio/matcher/internal/exception/services/command |
89.0% |
github.com/LerianStudio/matcher/internal/exception/services/query |
99.0% |
github.com/LerianStudio/matcher/internal/governance/adapters/audit |
91.8% |
github.com/LerianStudio/matcher/internal/governance/adapters/http/dto |
100.0% |
github.com/LerianStudio/matcher/internal/governance/adapters/http |
92.3% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres/actor_mapping |
97.5% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres/archive_metadata |
83.3% |
github.com/LerianStudio/matcher/internal/governance/adapters/postgres |
95.7% |
github.com/LerianStudio/matcher/internal/governance/domain/entities |
97.6% |
github.com/LerianStudio/matcher/internal/governance/domain/hashchain |
88.5% |
github.com/LerianStudio/matcher/internal/governance/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/governance/services/command |
91.1% |
github.com/LerianStudio/matcher/internal/governance/services/query |
100.0% |
github.com/LerianStudio/matcher/internal/governance/services/worker |
84.1% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/http/dto |
80.0% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/http |
94.2% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/parsers |
95.9% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/common |
67.8% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/job |
95.7% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/postgres/transaction |
95.7% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/rabbitmq |
77.6% |
github.com/LerianStudio/matcher/internal/ingestion/adapters/redis |
83.5% |
github.com/LerianStudio/matcher/internal/ingestion/domain/entities |
96.8% |
github.com/LerianStudio/matcher/internal/ingestion/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/ingestion/domain/value_objects |
97.1% |
github.com/LerianStudio/matcher/internal/ingestion/services/command |
86.0% |
github.com/LerianStudio/matcher/internal/ingestion/services/query |
86.7% |
github.com/LerianStudio/matcher/internal/matching/adapters/http/dto |
95.0% |
github.com/LerianStudio/matcher/internal/matching/adapters/http |
91.9% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/adjustment |
95.1% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/exception_creator |
95.8% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/fee_schedule |
93.6% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/fee_variance |
95.3% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_group |
90.4% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_item |
96.2% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/match_run |
96.5% |
github.com/LerianStudio/matcher/internal/matching/adapters/postgres/rate |
99.2% |
github.com/LerianStudio/matcher/internal/matching/adapters/rabbitmq |
74.4% |
github.com/LerianStudio/matcher/internal/matching/adapters/redis |
92.8% |
github.com/LerianStudio/matcher/internal/matching/domain/entities |
98.6% |
github.com/LerianStudio/matcher/internal/matching/domain/enums |
100.0% |
github.com/LerianStudio/matcher/internal/matching/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/matching/domain/services |
88.0% |
github.com/LerianStudio/matcher/internal/matching/domain/value_objects |
100.0% |
github.com/LerianStudio/matcher/internal/matching/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/matching/services/command |
86.7% |
github.com/LerianStudio/matcher/internal/matching/services/query |
94.1% |
github.com/LerianStudio/matcher/internal/outbox/domain/entities |
100.0% |
github.com/LerianStudio/matcher/internal/outbox/services |
84.5% |
github.com/LerianStudio/matcher/internal/reporting/adapters/http/dto |
82.2% |
github.com/LerianStudio/matcher/internal/reporting/adapters/http |
91.5% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/dashboard |
92.3% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/export_job |
94.5% |
github.com/LerianStudio/matcher/internal/reporting/adapters/postgres/report |
88.4% |
github.com/LerianStudio/matcher/internal/reporting/adapters/redis |
89.9% |
github.com/LerianStudio/matcher/internal/reporting/adapters/storage |
85.8% |
github.com/LerianStudio/matcher/internal/reporting/domain/entities |
97.7% |
github.com/LerianStudio/matcher/internal/reporting/domain/repositories/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/reporting/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/reporting/services/command |
98.6% |
github.com/LerianStudio/matcher/internal/reporting/services/query/exports |
83.0% |
github.com/LerianStudio/matcher/internal/reporting/services/query |
87.4% |
github.com/LerianStudio/matcher/internal/reporting/services/worker |
86.8% |
github.com/LerianStudio/matcher/internal/shared/adapters/cross |
90.2% |
github.com/LerianStudio/matcher/internal/shared/adapters/http |
83.8% |
github.com/LerianStudio/matcher/internal/shared/adapters/postgres/common |
87.8% |
github.com/LerianStudio/matcher/internal/shared/adapters/postgres/outbox |
76.7% |
github.com/LerianStudio/matcher/internal/shared/adapters/rabbitmq |
92.4% |
github.com/LerianStudio/matcher/internal/shared/domain/exception |
95.4% |
github.com/LerianStudio/matcher/internal/shared/domain/fee |
93.4% |
github.com/LerianStudio/matcher/internal/shared/domain |
98.2% |
github.com/LerianStudio/matcher/internal/shared/infrastructure/metrics |
94.7% |
github.com/LerianStudio/matcher/internal/shared/infrastructure/testutil |
45.8% |
github.com/LerianStudio/matcher/internal/shared/ports/mocks |
0.0% |
github.com/LerianStudio/matcher/internal/shared/ports |
33.8% |
github.com/LerianStudio/matcher/internal/shared/sanitize |
96.0% |
github.com/LerianStudio/matcher/internal/shared/testutil |
98.6% |
github.com/LerianStudio/matcher/internal/shared/utils |
100.0% |
github.com/LerianStudio/matcher/internal/testutil |
95.4% |
github.com/LerianStudio/matcher/pkg/chanutil |
100.0% |
github.com/LerianStudio/matcher/pkg/storageopt |
100.0% |
github.com/LerianStudio/matcher/tests/chaos |
84.1% |
Generated by Go PR Analysis workflow
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/bootstrap/systemplane_keys_tenancy.go (1)
122-177:⚠️ Potential issue | 🟠 MajorThese Redis Pub/Sub keys still over-promise runtime mutability.
All four
tenancy.multi_tenant_redis_*defs are markedApplyBundleRebuild+MutableAtRuntime, but the provided wiring only initializes event discovery once ininternal/bootstrap/init.go:1443-1482;internal/bootstrap/dynamic_infrastructure_provider.go:310-361only tears it down on provider close. Updating these values at runtime will not reconnect the listener, so the config contract here is inaccurate. Either add a reconciler that re-runsinitTenantEventDiscoveryon bundle rebuild or mark these keys bootstrap-only.♻️ Safe fallback if a reconciler is out of scope
- ApplyBehavior: domain.ApplyBundleRebuild, - MutableAtRuntime: true, + ApplyBehavior: domain.ApplyBootstrapOnly, + MutableAtRuntime: false,Apply the same change to all four
tenancy.multi_tenant_redis_*key defs.As per coding guidelines: "
internal/bootstrap/**/*.go: Bounded Context Bootstrap must implement App configuration (zero-config defaults + env overrides), dependency injection, server lifecycle, infrastructure connections (PostgreSQL primary/replica, Redis, RabbitMQ, S3), systemplane integration, dynamic infrastructure switching, worker lifecycle management, health checks, rate limiting, and observability (OpenTelemetry)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/systemplane_keys_tenancy.go` around lines 122 - 177, The four tenancy.multi_tenant_redis_* KeyDefs returned by matcherKeyDefsTenancyConnectivityRedis are currently marked ApplyBundleRebuild + MutableAtRuntime but the system only initializes tenant event discovery once via initTenantEventDiscovery (and only tears it down on provider close), so runtime updates won't reconnect listeners; fix by making these keys bootstrap-only: in matcherKeyDefsTenancyConnectivityRedis update each of the four defs to set MutableAtRuntime to false and change ApplyBehavior from domain.ApplyBundleRebuild to the bootstrap-only apply behavior (e.g., domain.ApplyBootstrapOnly) so the config contract matches actual wiring, and apply this change to all four keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/bootstrap/systemplane_keys_tenancy_test.go`:
- Around line 162-191: The test
TestMatcherKeyDefsTenancyConnectivityRedis_KeyProperties currently checks common
metadata but doesn't assert that the Redis password key preserves secrecy; add a
focused subtest (or an assertion inside the loop) that locates the def where
def.Key == "tenancy.multi_tenant_redis_password" (from
matcherKeyDefsTenancyConnectivityRedis), then assert def.IsSecret is true and
def.Redact equals domain.RedactFull (mirroring the pattern used in
TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret) to lock in the
secret/redaction contract.
In `@internal/bootstrap/tenant_event_discovery.go`:
- Around line 43-47: Update initTenantEventDiscovery to be context-aware: change
its signature to accept ctx context.Context and return (
*tmevent.TenantEventListener, *tenantcache.TenantCache, func() error ). Inside
initTenantEventDiscovery replace any use of context.Background() when dialing
Redis, starting the listener, or logging shutdown with the provided ctx, and
make the teardown closure perform cancellable/checked shutdown steps that
propagate errors (return error) instead of swallowing them. Ensure any calls to
methods on tmevent.TenantEventListener or tenantcache.TenantCache that support
context use ctx (e.g., listener start/stop or Redis dial functions) and update
all callers of initTenantEventDiscovery to pass a context and handle the new
func() error return type.
---
Duplicate comments:
In `@internal/bootstrap/systemplane_keys_tenancy.go`:
- Around line 122-177: The four tenancy.multi_tenant_redis_* KeyDefs returned by
matcherKeyDefsTenancyConnectivityRedis are currently marked ApplyBundleRebuild +
MutableAtRuntime but the system only initializes tenant event discovery once via
initTenantEventDiscovery (and only tears it down on provider close), so runtime
updates won't reconnect listeners; fix by making these keys bootstrap-only: in
matcherKeyDefsTenancyConnectivityRedis update each of the four defs to set
MutableAtRuntime to false and change ApplyBehavior from
domain.ApplyBundleRebuild to the bootstrap-only apply behavior (e.g.,
domain.ApplyBootstrapOnly) so the config contract matches actual wiring, and
apply this change to all four keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1a2b0f3-5840-49d2-941a-c3e6457fa9c4
📒 Files selected for processing (11)
internal/bootstrap/systemplane_keys_tenancy.gointernal/bootstrap/systemplane_keys_tenancy_test.gointernal/bootstrap/tenant_event_discovery.gointernal/exception/adapters/redis/callback_rate_limiter_test.gointernal/ingestion/adapters/rabbitmq/event_publisher.gointernal/ingestion/adapters/rabbitmq/event_publisher_test.gointernal/ingestion/adapters/redis/dedupe_service_test.gointernal/matching/adapters/rabbitmq/event_publisher.gointernal/matching/adapters/rabbitmq/event_publisher_test.gointernal/reporting/adapters/redis/cache_service_test.gointernal/reporting/adapters/storage/s3_client.go
Comment on lines
+162
to
+191
| func TestMatcherKeyDefsTenancyConnectivityRedis_KeyProperties(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| defs := matcherKeyDefsTenancyConnectivityRedis() | ||
|
|
||
| expectedKeys := []string{ | ||
| "tenancy.multi_tenant_redis_host", | ||
| "tenancy.multi_tenant_redis_port", | ||
| "tenancy.multi_tenant_redis_password", | ||
| "tenancy.multi_tenant_redis_tls", | ||
| } | ||
|
|
||
| require.Len(t, defs, len(expectedKeys)) | ||
|
|
||
| for i, expKey := range expectedKeys { | ||
| t.Run(expKey, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| def := defs[i] | ||
| assert.Equal(t, expKey, def.Key) | ||
| assert.Equal(t, domain.KindConfig, def.Kind) | ||
| assert.Equal(t, "tenancy", def.Group) | ||
| assert.Equal(t, domain.ApplyBundleRebuild, def.ApplyBehavior) | ||
| assert.True(t, def.MutableAtRuntime) | ||
| assert.NotEmpty(t, def.Description) | ||
| require.Len(t, def.AllowedScopes, 1) | ||
| assert.Equal(t, domain.ScopeGlobal, def.AllowedScopes[0]) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Lock in the Redis password secrecy contract with a dedicated test.
The new loop checks common metadata, but it will not pinpoint regressions if tenancy.multi_tenant_redis_password loses Secret or RedactFull. A focused assertion here—mirroring TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret—would keep the credential-handling contract explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/systemplane_keys_tenancy_test.go` around lines 162 - 191,
The test TestMatcherKeyDefsTenancyConnectivityRedis_KeyProperties currently
checks common metadata but doesn't assert that the Redis password key preserves
secrecy; add a focused subtest (or an assertion inside the loop) that locates
the def where def.Key == "tenancy.multi_tenant_redis_password" (from
matcherKeyDefsTenancyConnectivityRedis), then assert def.IsSecret is true and
def.Redact equals domain.RedactFull (mirroring the pattern used in
TestMatcherKeyDefsTenancyResilience_ServiceAPIKeyIsSecret) to lock in the
secret/redaction contract.
Comment on lines
+43
to
+47
| func initTenantEventDiscovery( | ||
| cfg *Config, | ||
| pgManager *tmpostgres.Manager, | ||
| logger libLog.Logger, | ||
| ) (*tmevent.TenantEventListener, *tenantcache.TenantCache, func()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make the event-discovery resource lifecycle-aware.
This path dials Redis, starts the listener, and logs shutdown failures with context.Background(), then exposes teardown as func(), so cancellation and cleanup errors cannot flow back through the bootstrap lifecycle. Please accept a caller ctx here and return func() error so startup/shutdown behavior matches the other provider resources.
As per coding guidelines: "internal/bootstrap/**/*.go: Bounded Context Bootstrap must implement App configuration (zero-config defaults + env overrides), dependency injection, server lifecycle, infrastructure connections (PostgreSQL primary/replica, Redis, RabbitMQ, S3), systemplane integration, dynamic infrastructure switching, worker lifecycle management, health checks, rate limiting, and observability (OpenTelemetry)"
Also applies to: 69-73, 100-107, 122-148, 155-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bootstrap/tenant_event_discovery.go` around lines 43 - 47, Update
initTenantEventDiscovery to be context-aware: change its signature to accept ctx
context.Context and return ( *tmevent.TenantEventListener,
*tenantcache.TenantCache, func() error ). Inside initTenantEventDiscovery
replace any use of context.Background() when dialing Redis, starting the
listener, or logging shutdown with the provided ctx, and make the teardown
closure perform cancellable/checked shutdown steps that propagate errors (return
error) instead of swallowing them. Ensure any calls to methods on
tmevent.TenantEventListener or tenantcache.TenantCache that support context use
ctx (e.g., listener start/stop or Redis dial functions) and update all callers
of initTenantEventDiscovery to pass a context and handle the new func() error
return type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
go build ./...passes🤖 Generated with Claude Code