feat(hub): replace redis subchart with self-managed Redis - #169
Open
maxju wants to merge 2 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughFlame Hub replaces its Redis dependency with an in-chart Redis deployment and configurable connectivity. Harbor settings gain validation. Flame Node updates service environment variables, Secret naming, URL validation, ingress values, and component image versions. ChangesFlame platform chart updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Flux Statusℹ️ HelmRelease/pr-169-hub/hub Helm install succeeded for release pr-169-hub/hub.v1 with chart flame-hub@0.1.0+00cde42b6595 Metadata:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/flame-hub/templates/_helpers.tpl`:
- Around line 94-96: Separate the consumer Redis host from the chart-managed
Service name: in charts/flame-hub/templates/_helpers.tpl lines 94-96, retain
flameHub.redis.host for connectivity and add a fixed chart-managed Service-name
helper; in charts/flame-hub/values.yaml lines 72-77, add and document
redis.enabled: false for external Redis; in
charts/flame-hub/templates/redis/service.yaml line 4 and
charts/flame-hub/templates/redis/statefulset.yaml line 8, use the new
Service-name helper and conditionally skip both resources when redis.enabled is
false.
In `@charts/flame-hub/templates/_urls.tpl`:
- Around line 175-178: Update the Harbor validation near $exposeType to also
reject configurations where harbor.expose.type is clusterIP and
harbor.expose.tls.enabled is false, since wait-for-harbor expects HTTPS.
Preserve the existing rejection for non-clusterIP exposure and provide a clear
failure message directing users to enable TLS or configure the init container
for the selected protocol.
In `@charts/third-party/authup/values.yaml`:
- Around line 41-44: Propagate the effective Redis port into Authup: add a
templatable redis.port value in charts/third-party/authup/values.yaml#L41-L44,
pass the configured Redis service port from
charts/flame-hub/values.yaml#L203-L205 in a way available to the Authup
subchart, and update both hardcoded 6379 references in
charts/third-party/authup/templates/deployment.yaml#L32 and `#L78` to use
redis.port.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc9fc807-8c58-4071-b05c-edb8a879c115
📒 Files selected for processing (21)
charts/flame-hub/Chart.yamlcharts/flame-hub/README.mdcharts/flame-hub/templates/_helpers.tplcharts/flame-hub/templates/_urls.tplcharts/flame-hub/templates/credentials-secret.yamlcharts/flame-hub/templates/harbor/secret.yamlcharts/flame-hub/templates/redis/service.yamlcharts/flame-hub/templates/redis/statefulset.yamlcharts/flame-hub/templates/server-core/deployment.yamlcharts/flame-hub/templates/server-messenger/deployment.yamlcharts/flame-hub/templates/server-storage/deployment.yamlcharts/flame-hub/templates/server-telemetry/deployment.yamlcharts/flame-hub/values.yamlcharts/flame-node/templates/_helpers.tplcharts/flame-node/templates/data-store/httproute.yamlcharts/flame-node/templates/hub-adapter/deployment.yamlcharts/flame-node/templates/secret.yamlcharts/flame-node/templates/storage-service/deployment.yamlcharts/flame-node/values.yamlcharts/third-party/authup/templates/deployment.yamlcharts/third-party/authup/values.yaml
💤 Files with no reviewable changes (1)
- charts/flame-hub/Chart.yaml
Comment on lines
+94
to
+96
| {{- define "flameHub.redis.host" -}} | ||
| {{- $redis := (((.Values.global).flameHub).redis) | default dict -}} | ||
| {{- $redis.host | default (printf "%s-redis" .Release.Name) -}} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Separate the Redis connection host from the chart-managed Service name.
If global.flameHub.redis.host is set to external-redis, Line 4 creates a local Service with that name and selectors for this chart’s Redis Pod. Consumers then resolve the local Service instead of the intended external Redis. If the external endpoint is an FQDN, Kubernetes rejects it as a Service name.
charts/flame-hub/templates/_helpers.tpl#L94-L96: keepflameHub.redis.hostfor consumer connectivity only. Add a separate fixed chart-managed Service-name helper.charts/flame-hub/values.yaml#L72-L77: add an explicit external-Redis mode, such asredis.enabled: false, and document that mode.charts/flame-hub/templates/redis/service.yaml#L4-L4: use the chart-managed Service-name helper. Do not render this Service for external Redis.charts/flame-hub/templates/redis/statefulset.yaml#L8-L8: use the chart-managed Service-name helper. Do not render this StatefulSet for external Redis.
📍 Affects 4 files
charts/flame-hub/templates/_helpers.tpl#L94-L96(this comment)charts/flame-hub/values.yaml#L72-L77charts/flame-hub/templates/redis/service.yaml#L4-L4charts/flame-hub/templates/redis/statefulset.yaml#L8-L8
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/flame-hub/templates/_helpers.tpl` around lines 94 - 96, Separate the
consumer Redis host from the chart-managed Service name: in
charts/flame-hub/templates/_helpers.tpl lines 94-96, retain flameHub.redis.host
for connectivity and add a fixed chart-managed Service-name helper; in
charts/flame-hub/values.yaml lines 72-77, add and document redis.enabled: false
for external Redis; in charts/flame-hub/templates/redis/service.yaml line 4 and
charts/flame-hub/templates/redis/statefulset.yaml line 8, use the new
Service-name helper and conditionally skip both resources when redis.enabled is
false.
Comment on lines
+41
to
+44
| redis: | ||
| # Redis host authup connects to. Rendered with tpl, so a parent chart may pass a | ||
| # helper, e.g. '{{ include "flameHub.redis.host" . }}'. Defaults to <release>-redis. | ||
| host: "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate the effective Redis port into Authup.
If redis.service.port differs from 6379, Hub workloads and redis-connection-string use the configured port, but Authup’s init container always waits on port 6379. Authup never starts in that configuration.
charts/third-party/authup/values.yaml#L41-L44: add a templatableredis.portvalue.charts/flame-hub/values.yaml#L203-L205: pass the effective Redis port through globally shared values or a helper that is valid in the Authup subchart context.charts/third-party/authup/templates/deployment.yaml#L32-L32: replace hardcoded6379withredis.port.charts/third-party/authup/templates/deployment.yaml#L78-L78: replace hardcoded6379withredis.port.
📍 Affects 3 files
charts/third-party/authup/values.yaml#L41-L44(this comment)charts/flame-hub/values.yaml#L203-L205charts/third-party/authup/templates/deployment.yaml#L32-L32charts/third-party/authup/templates/deployment.yaml#L78-L78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/third-party/authup/values.yaml` around lines 41 - 44, Propagate the
effective Redis port into Authup: add a templatable redis.port value in
charts/third-party/authup/values.yaml#L41-L44, pass the configured Redis service
port from charts/flame-hub/values.yaml#L203-L205 in a way available to the
Authup subchart, and update both hardcoded 6379 references in
charts/third-party/authup/templates/deployment.yaml#L32 and `#L78` to use
redis.port.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/flame-hub/templates/_helpers.tpl`:
- Around line 94-96: Separate the consumer Redis host from the chart-managed
Service name: in charts/flame-hub/templates/_helpers.tpl lines 94-96, retain
flameHub.redis.host for connectivity and add a fixed chart-managed Service-name
helper; in charts/flame-hub/values.yaml lines 72-77, add and document
redis.enabled: false for external Redis; in
charts/flame-hub/templates/redis/service.yaml line 4 and
charts/flame-hub/templates/redis/statefulset.yaml line 8, use the new
Service-name helper and conditionally skip both resources when redis.enabled is
false.
In `@charts/flame-hub/templates/_urls.tpl`:
- Around line 175-178: Update the Harbor validation near $exposeType to also
reject configurations where harbor.expose.type is clusterIP and
harbor.expose.tls.enabled is false, since wait-for-harbor expects HTTPS.
Preserve the existing rejection for non-clusterIP exposure and provide a clear
failure message directing users to enable TLS or configure the init container
for the selected protocol.
In `@charts/third-party/authup/values.yaml`:
- Around line 41-44: Propagate the effective Redis port into Authup: add a
templatable redis.port value in charts/third-party/authup/values.yaml#L41-L44,
pass the configured Redis service port from
charts/flame-hub/values.yaml#L203-L205 in a way available to the Authup
subchart, and update both hardcoded 6379 references in
charts/third-party/authup/templates/deployment.yaml#L32 and `#L78` to use
redis.port.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc9fc807-8c58-4071-b05c-edb8a879c115
📒 Files selected for processing (21)
charts/flame-hub/Chart.yamlcharts/flame-hub/README.mdcharts/flame-hub/templates/_helpers.tplcharts/flame-hub/templates/_urls.tplcharts/flame-hub/templates/credentials-secret.yamlcharts/flame-hub/templates/harbor/secret.yamlcharts/flame-hub/templates/redis/service.yamlcharts/flame-hub/templates/redis/statefulset.yamlcharts/flame-hub/templates/server-core/deployment.yamlcharts/flame-hub/templates/server-messenger/deployment.yamlcharts/flame-hub/templates/server-storage/deployment.yamlcharts/flame-hub/templates/server-telemetry/deployment.yamlcharts/flame-hub/values.yamlcharts/flame-node/templates/_helpers.tplcharts/flame-node/templates/data-store/httproute.yamlcharts/flame-node/templates/hub-adapter/deployment.yamlcharts/flame-node/templates/secret.yamlcharts/flame-node/templates/storage-service/deployment.yamlcharts/flame-node/values.yamlcharts/third-party/authup/templates/deployment.yamlcharts/third-party/authup/values.yaml
💤 Files with no reviewable changes (1)
- charts/flame-hub/Chart.yaml
🛑 Comments failed to post (1)
charts/flame-hub/templates/_urls.tpl (1)
175-178: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject HTTP-only Harbor configurations.
If
harbor.expose.typeisclusterIPandharbor.expose.tls.enabledisfalse, this validation passes. Thewait-for-harborinit container always callsharbor.internalHostwithhttps://(Lines 56-62 incharts/flame-hub/templates/server-core/deployment.yaml). (raw.githubusercontent.com) The pod then waits forever.Fail when TLS is disabled, or make the init container use the configured protocol.
Proposed validation
{{- if ne $exposeType "clusterIP" -}} {{- fail (printf "harbor.expose.type=%q is not supported by this chart: harbor.internalHost (used by the wait-for-harbor init container and in-cluster access) only resolves the clusterIP Service. Set harbor.expose.type=clusterIP, or use externalHarbor." $exposeType) -}} {{- end -}} +{{- if not .Values.harbor.expose.tls.enabled -}} +{{- fail "harbor.expose.tls.enabled must be true when Harbor is accessed through harbor.internalHost." -}} +{{- end -}}📝 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.{{- $exposeType := .Values.harbor.expose.type | default "clusterIP" -}} {{- if ne $exposeType "clusterIP" -}} {{- fail (printf "harbor.expose.type=%q is not supported by this chart: harbor.internalHost (used by the wait-for-harbor init container and in-cluster access) only resolves the clusterIP Service. Set harbor.expose.type=clusterIP, or use externalHarbor." $exposeType) -}} {{- end -}} {{- if not .Values.harbor.expose.tls.enabled -}} {{- fail "harbor.expose.tls.enabled must be true when Harbor is accessed through harbor.internalHost." -}} {{- end -}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/flame-hub/templates/_urls.tpl` around lines 175 - 178, Update the Harbor validation near $exposeType to also reject configurations where harbor.expose.type is clusterIP and harbor.expose.tls.enabled is false, since wait-for-harbor expects HTTPS. Preserve the existing rejection for non-clusterIP exposure and provide a clear failure message directing users to enable TLS or configure the init container for the selected protocol.
Flux Statusℹ️ HelmRelease/pr-169-node-bunny/node Helm install succeeded for release pr-169-node-bunny/node.v1 with chart flame-node@0.1.0+00cde42b6595 Metadata:
|
Flux Statusℹ️ HelmRelease/pr-169-node-bear/node Helm install succeeded for release pr-169-node-bear/node.v1 with chart flame-node@0.1.0+00cde42b6595 Metadata:
|
maxju
added
deploy/preview
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Chores