-
Notifications
You must be signed in to change notification settings - Fork 3
fix(helm): align Novu services Redis DB index to 2 #1196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
All Novu services (API, Worker, WS, Embed) were using inconsistent Redis DB index values, causing in-app notifications to fail delivery. The Worker publishes notifications to Redis, but the WS service was listening on a different DB index, preventing real-time notification delivery to the frontend. Changes: - Updated api/deployment.yaml to use configurable REDIS_DB_INDEX with default "2" - Updated worker/deployment.yaml to use configurable REDIS_DB_INDEX with default "2" - Updated ws/deployment.yaml to use configurable REDIS_DB_INDEX with default "2" - Updated extra/novu.yaml embed service to use REDIS_DB_INDEX "2" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @dittops, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where in-app notifications were not being delivered due to inconsistent Redis database index configurations across Novu services. By aligning the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly identifies and fixes a critical bug where Novu services were using inconsistent Redis DB indexes, preventing real-time notifications. The changes align the services to use DB index 2.
My review focuses on improving the maintainability and robustness of this fix. I've suggested centralizing the REDIS_DB_INDEX configuration to prevent similar inconsistencies in the future. Specifically:
- For the
workerandwsservices, I recommend using the same configuration value fromapi.envsince they already share other environment variables from there. - For the
embedservice, I suggest parameterizing the hardcoded value to also use this centralized configuration.
These changes will create a single source of truth for the Redis DB index across all Novu services, making the system easier to manage and less prone to configuration errors.
| value : {{ include "novu.redis.port" . | quote }} | ||
| - name: REDIS_DB_INDEX | ||
| value : "1" | ||
| value : {{ .Values.worker.env.REDIS_DB_INDEX | default "2" | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure all services consistently use the same Redis DB index and to prevent future misconfigurations, it's best to use a single source of truth. Since this service already uses environment variables from api.env (like NODE_ENV), I recommend using .Values.api.env.REDIS_DB_INDEX here as well. This avoids introducing a new, separate configuration path (.Values.worker.env) that could diverge.
value : {{ .Values.api.env.REDIS_DB_INDEX | default "2" | quote }}| value : {{ include "novu.redis.port" . | quote }} | ||
| - name: REDIS_DB_INDEX | ||
| value : "1" | ||
| value : {{ .Values.ws.env.REDIS_DB_INDEX | default "2" | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure all services consistently use the same Redis DB index and to prevent future misconfigurations, it's best to use a single source of truth. Since this service already uses environment variables from api.env (like NODE_ENV), I recommend using .Values.api.env.REDIS_DB_INDEX here as well. This avoids introducing a new, separate configuration path (.Values.ws.env) that could diverge.
value : {{ .Values.api.env.REDIS_DB_INDEX | default "2" | quote }}| value : "6379" | ||
| - name: REDIS_DB_INDEX | ||
| value : "1" | ||
| value : "2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While changing the value to 2 fixes the immediate issue, hardcoding it makes future maintenance difficult. To ensure this embed service stays aligned with the other Novu services, I recommend parameterizing this value to use the same configuration source. This creates a single point of change for the Redis DB index.
Assuming the novu subchart is referenced with the name novu, you can use its value like this:
value : {{ .Values.novu.api.env.REDIS_DB_INDEX | default "2" | quote }}Addresses Gemini Code Assist review feedback to centralize the REDIS_DB_INDEX configuration to prevent future inconsistencies. Changes: - Worker and WS deployments now use .Values.api.env.REDIS_DB_INDEX instead of service-specific values, matching existing pattern where they already share api.env variables like NODE_ENV - Embed service now uses .Values.novu.api.env.REDIS_DB_INDEX to stay aligned with the Novu subchart configuration This ensures all Novu services use a single configuration source for Redis DB index, making the system easier to manage and less prone to configuration drift. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Changes
infra/helm/bud/charts/novu/templates/api/deployment.yaml- configurable REDIS_DB_INDEX with default "2"infra/helm/bud/charts/novu/templates/worker/deployment.yaml- configurable REDIS_DB_INDEX with default "2"infra/helm/bud/charts/novu/templates/ws/deployment.yaml- configurable REDIS_DB_INDEX with default "2"infra/helm/bud/templates/extra/novu.yaml- embed service REDIS_DB_INDEX set to "2"Test plan
🤖 Generated with Claude Code