Conversation
# Conflicts: # charts/flame-node/values.yaml
# Conflicts: # charts/flame-node/templates/pod-orchestrator/deployment.yaml
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughSwitches the Helm chart’s Hub authentication from robot-based to client-based credentials across multiple templates, updates secret keys and env var names, bumps several service image tags, adds two NGINX ingress annotations, and updates the keycloakx dependency version. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
charts/flame-node/templates/secret.yaml (1)
20-24: Hardcoded default credentials in secrets.These base64-encoded credentials (
admin,flame_user,flame_password,postgres_db) are committed to the repository. While these may be intended as development defaults, consider:
- Adding a clear comment that these must be overridden in production
- Documenting in values.yaml that users should provide their own secrets via
existingSecretreferences for production deployments🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-node/templates/secret.yaml` around lines 20 - 24, The secret template contains hardcoded base64 credentials (data.superPassword, data.DB_USER, data.DB_PASSWORD, data.DB_DATABASE); update the template to remove committed defaults and instead reference values (e.g. .Values.secrets.superPassword, .Values.secrets.DB_USER, etc.) and/or an existingSecret reference (existingSecret) so production can supply secrets, and add a clear comment near those keys stating "MUST be overridden in production" and document in values.yaml that users must provide their own secrets or set existingSecret for production deployments.charts/flame-node/templates/ui/ingress.yaml (1)
6-11: Hardcoded NGINX-specific annotations assume NGINX Ingress Controller.The buffer size annotations are NGINX Ingress Controller-specific. If users deploy with a different ingress controller (Traefik, HAProxy, etc.), these annotations will be ignored. Consider making these configurable via
values.yamlto support different ingress controllers and allow tuning.♻️ Optional: Make buffer sizes configurable
annotations: - nginx.ingress.kubernetes.io/proxy-buffer-size: "128k" - nginx.ingress.kubernetes.io/client-body-buffer-size: "32k" + {{- if .Values.ingress.nginx.enabled | default true }} + nginx.ingress.kubernetes.io/proxy-buffer-size: {{ .Values.ingress.nginx.proxyBufferSize | default "128k" | quote }} + nginx.ingress.kubernetes.io/client-body-buffer-size: {{ .Values.ingress.nginx.clientBodyBufferSize | default "32k" | quote }} + {{- end }} {{- with .Values.ingress.annotations }} {{- toYaml . | nindent 4 }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-node/templates/ui/ingress.yaml` around lines 6 - 11, The chart currently hardcodes NGINX-specific annotations in the annotations block (see annotations: and the existing {{- with .Values.ingress.annotations }} usage), which should be configurable; update values.yaml to expose either controller-specific keys (e.g., ingress.nginx.bufferSize and ingress.nginx.clientBodyBufferSize) or a generic ingress.controllerAnnotations map, then change templates/ui/ingress.yaml to render NGINX annotations only when those values are set or when .Values.ingress.controller == "nginx", and merge them with the existing .Values.ingress.annotations so users can supply Traefik/HAProxy annotations instead; ensure the template uses conditional logic (if/with) to avoid emitting empty or irrelevant controller annotations.charts/flame-node/templates/_helpers.tpl (1)
47-57: Comment and fallback secret name still reference "robot" terminology.The helper variable was renamed to
$clientSecretName, but the comment on line 48 ("Return the secret containing the hub robot secret") and the fallback secret name on line 55 (hub-robot-secret) still use "robot" terminology. Consider updating for consistency with the new client-based authentication model.Note: Changing the fallback secret name would be a breaking change for existing deployments, so keeping it as-is may be intentional for backward compatibility.
📝 Suggested comment update (non-breaking)
{{/* -Return the secret containing the hub robot secret +Return the secret containing the hub client credentials */}} {{- define "hub.secretName" -}} {{- $clientSecretName := .Values.hub.auth.existingSecret -}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-node/templates/_helpers.tpl` around lines 47 - 57, Update the comment and, optionally, the fallback secret name in the "hub.secretName" helper to reflect the client-based auth naming: change the comment that currently reads "Return the secret containing the hub robot secret" to reference the hub client secret, and if you choose to rename the fallback for consistency, replace the literal "hub-robot-secret" used in the else branch with "hub-client-secret"; if you must preserve backward compatibility, leave the fallback string unchanged but still update the comment; locate the logic by the define "hub.secretName" and the variable $clientSecretName.charts/flame-node/templates/hub-adapter/deployment.yaml (1)
46-56: Consider pinning the curl image tag for reproducibility.The initContainer uses
curlimages/curl:latestwhich can lead to inconsistent behavior across deployments if the image is updated. Pin to a specific version for consistency.📌 Proposed fix
initContainers: - name: wait-for-keycloak - image: curlimages/curl:latest + image: curlimages/curl:8.18.0 imagePullPolicy: IfNotPresent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-node/templates/hub-adapter/deployment.yaml` around lines 46 - 56, The initContainer "wait-for-keycloak" currently uses the floating tag curlimages/curl:latest; change it to a pinned, specific version (for example curlimages/curl:8.4.0 or another vetted semver) to ensure reproducible deployments; update the image reference in the deployment template where "wait-for-keycloak" and image: curlimages/curl:latest appear and verify imagePullPolicy remains IfNotPresent.charts/flame-node/values.yaml (1)
490-491: Replace release TODO with tracked issue/reference.Keeping a TODO in default chart values makes release intent ambiguous. Prefer a concrete issue link or remove the TODO once validated.
If you want, I can draft a small follow-up patch to replace this with a version-compatibility note plus issue reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-node/values.yaml` around lines 490 - 491, Replace the transient TODO comment above the "tag: 0.4.0" entry with a concrete reference or remove it: either replace the TODO with a short compatibility note plus a tracked issue/PR URL (e.g., "See ISSUE-1234 for upgrade validation") or delete the TODO entirely now that the version is validated; update the comment immediately above the tag key so the value "tag: 0.4.0" remains but no ambiguous TODO remains in the chart defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/flame-node/templates/pod-orchestrator/deployment.yaml`:
- Around line 112-118: Update the required message for the HUB_CLIENT_ID
environment variable to reference the client ID instead of "robot ID": in the
deployment template where the env var name HUB_CLIENT_ID is set (and next to
HUB_CLIENT_SECRET and include "hub.secretName"), replace the required message "A
robot ID for the Hub is required." with a consistent message like "A client ID
for the Hub is required." so the error text matches the HUB_CLIENT_ID variable.
In `@charts/flame-node/values.yaml`:
- Around line 98-101: Remove the unused legacy auth keys by deleting
HUB_AUTH_ROBOT_ID and HUB_AUTH_ROBOT_SECRET from values.yaml so only the
actively used hub auth fields remain; update the values schema/comments to
reflect that templates reference .Values.hub.auth.clientID and
.Values.hub.auth.clientSecret (and the optional hub.auth.existingSecret) to
avoid confusion and misconfiguration.
---
Nitpick comments:
In `@charts/flame-node/templates/_helpers.tpl`:
- Around line 47-57: Update the comment and, optionally, the fallback secret
name in the "hub.secretName" helper to reflect the client-based auth naming:
change the comment that currently reads "Return the secret containing the hub
robot secret" to reference the hub client secret, and if you choose to rename
the fallback for consistency, replace the literal "hub-robot-secret" used in the
else branch with "hub-client-secret"; if you must preserve backward
compatibility, leave the fallback string unchanged but still update the comment;
locate the logic by the define "hub.secretName" and the variable
$clientSecretName.
In `@charts/flame-node/templates/hub-adapter/deployment.yaml`:
- Around line 46-56: The initContainer "wait-for-keycloak" currently uses the
floating tag curlimages/curl:latest; change it to a pinned, specific version
(for example curlimages/curl:8.4.0 or another vetted semver) to ensure
reproducible deployments; update the image reference in the deployment template
where "wait-for-keycloak" and image: curlimages/curl:latest appear and verify
imagePullPolicy remains IfNotPresent.
In `@charts/flame-node/templates/secret.yaml`:
- Around line 20-24: The secret template contains hardcoded base64 credentials
(data.superPassword, data.DB_USER, data.DB_PASSWORD, data.DB_DATABASE); update
the template to remove committed defaults and instead reference values (e.g.
.Values.secrets.superPassword, .Values.secrets.DB_USER, etc.) and/or an
existingSecret reference (existingSecret) so production can supply secrets, and
add a clear comment near those keys stating "MUST be overridden in production"
and document in values.yaml that users must provide their own secrets or set
existingSecret for production deployments.
In `@charts/flame-node/templates/ui/ingress.yaml`:
- Around line 6-11: The chart currently hardcodes NGINX-specific annotations in
the annotations block (see annotations: and the existing {{- with
.Values.ingress.annotations }} usage), which should be configurable; update
values.yaml to expose either controller-specific keys (e.g.,
ingress.nginx.bufferSize and ingress.nginx.clientBodyBufferSize) or a generic
ingress.controllerAnnotations map, then change templates/ui/ingress.yaml to
render NGINX annotations only when those values are set or when
.Values.ingress.controller == "nginx", and merge them with the existing
.Values.ingress.annotations so users can supply Traefik/HAProxy annotations
instead; ensure the template uses conditional logic (if/with) to avoid emitting
empty or irrelevant controller annotations.
In `@charts/flame-node/values.yaml`:
- Around line 490-491: Replace the transient TODO comment above the "tag: 0.4.0"
entry with a concrete reference or remove it: either replace the TODO with a
short compatibility note plus a tracked issue/PR URL (e.g., "See ISSUE-1234 for
upgrade validation") or delete the TODO entirely now that the version is
validated; update the comment immediately above the tag key so the value "tag:
0.4.0" remains but no ambiguous TODO remains in the chart defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c19b0fd7-0816-4176-83c2-5de389392d5f
📒 Files selected for processing (9)
charts/flame-node/Chart.yamlcharts/flame-node/templates/_helpers.tplcharts/flame-node/templates/hub-adapter/deployment.yamlcharts/flame-node/templates/message-broker/deployment.ymlcharts/flame-node/templates/pod-orchestrator/deployment.yamlcharts/flame-node/templates/secret.yamlcharts/flame-node/templates/storage-service/deployment.yamlcharts/flame-node/templates/ui/ingress.yamlcharts/flame-node/values.yaml
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
2 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.
Node templates and images were updated to now support Hub >= v0.8.31
Summary by CodeRabbit
Chores
New Features