feat: add optional gRPC port to bifrost cluster deployment and service#3617
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR conditionally exposes a gRPC TCP port when ChangesgRPC Port Exposure
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
force-pushed
the
05-20-chore_add_timeout_example_in_providers_yaml_file
branch
from
06e985b to
f4a7c96
Compare
BearTS
force-pushed
the
05-20-chore_expose_grpc_service_port_if_mentioned
branch
from
5405e4c to
36a1311
Compare
BearTS
changed the title
Confidence Score: 5/5The changes are additive, scoped to Helm templates, and consistent with the identical pattern already shipped in stateful.yaml and service-headless.yaml. Both modified templates follow the same guard and naming conventions already established in the rest of the chart. The gRPC port is only rendered inside the existing cluster-enabled block, the targetPort reference correctly matches the named container port, and no existing ports are affected. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "chore: expose grpc service port if menti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
helm-charts/bifrost/templates/deployment.yaml (1)
79-83: ⚡ Quick winConsider validating that the port value exists.
The conditional checks if
.Values.bifrost.cluster.grpcis truthy but then accesses.Values.bifrost.cluster.grpc.portwithout verifying theportfield exists. If a user setsbifrost.cluster.grpc: {}or defines grpc configuration without the port field, the template will fail with an unclear error during rendering.While this follows the existing pattern used for gossip ports (lines 73-78), consider one of these approaches for better user experience:
-{{- if .Values.bifrost.cluster.grpc }} +{{- if .Values.bifrost.cluster.grpc.port }}or with an explicit validation message:
-{{- if .Values.bifrost.cluster.grpc }} - name: grpc - containerPort: {{ .Values.bifrost.cluster.grpc.port }} + containerPort: {{ .Values.bifrost.cluster.grpc.port | required "bifrost.cluster.grpc.port is required when grpc is enabled" }} protocol: TCP -{{- 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 `@helm-charts/bifrost/templates/deployment.yaml` around lines 79 - 83, The template currently checks .Values.bifrost.cluster.grpc but dereferences .Values.bifrost.cluster.grpc.port which can be missing; update the deployment template to validate that the port field exists (e.g., check both .Values.bifrost.cluster.grpc and .Values.bifrost.cluster.grpc.port or use default/required logic) before rendering the grpc containerPort, and provide a clear error or fallback value if port is absent; locate the grpc block in the deployment template (the conditional using .Values.bifrost.cluster.grpc and the reference to .Values.bifrost.cluster.grpc.port) and adjust the conditional or add a validation message so the template won’t fail when grpc is defined without a port.helm-charts/bifrost/templates/service.yaml (1)
28-33: ⚡ Quick winConsider validating that the port value exists.
The conditional checks if
.Values.bifrost.cluster.grpcis truthy but then accesses.Values.bifrost.cluster.grpc.portwithout verifying theportfield exists. This mirrors the same concern in the deployment template and could lead to unclear template rendering errors if misconfigured.For consistency with the deployment template fix, consider the same validation approach:
-{{- if .Values.bifrost.cluster.grpc }} +{{- if .Values.bifrost.cluster.grpc.port }}or with an explicit validation message:
-{{- if .Values.bifrost.cluster.grpc }} -- port: {{ .Values.bifrost.cluster.grpc.port }} +- port: {{ .Values.bifrost.cluster.grpc.port | required "bifrost.cluster.grpc.port is required when grpc is enabled" }} targetPort: grpc protocol: TCP name: grpc -{{- 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 `@helm-charts/bifrost/templates/service.yaml` around lines 28 - 33, The service template uses .Values.bifrost.cluster.grpc.port without ensuring the port field exists; update the conditional around the grpc port block in service.yaml to only render when both .Values.bifrost.cluster.grpc and .Values.bifrost.cluster.grpc.port are present (or explicitly fail with a clear message), so the port/targetPort/name grpc stanza is omitted or validated if port is missing; target the .Values.bifrost.cluster.grpc and .Values.bifrost.cluster.grpc.port symbols and the grpc port block when making the change.
🤖 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.
Nitpick comments:
In `@helm-charts/bifrost/templates/deployment.yaml`:
- Around line 79-83: The template currently checks .Values.bifrost.cluster.grpc
but dereferences .Values.bifrost.cluster.grpc.port which can be missing; update
the deployment template to validate that the port field exists (e.g., check both
.Values.bifrost.cluster.grpc and .Values.bifrost.cluster.grpc.port or use
default/required logic) before rendering the grpc containerPort, and provide a
clear error or fallback value if port is absent; locate the grpc block in the
deployment template (the conditional using .Values.bifrost.cluster.grpc and the
reference to .Values.bifrost.cluster.grpc.port) and adjust the conditional or
add a validation message so the template won’t fail when grpc is defined without
a port.
In `@helm-charts/bifrost/templates/service.yaml`:
- Around line 28-33: The service template uses .Values.bifrost.cluster.grpc.port
without ensuring the port field exists; update the conditional around the grpc
port block in service.yaml to only render when both .Values.bifrost.cluster.grpc
and .Values.bifrost.cluster.grpc.port are present (or explicitly fail with a
clear message), so the port/targetPort/name grpc stanza is omitted or validated
if port is missing; target the .Values.bifrost.cluster.grpc and
.Values.bifrost.cluster.grpc.port symbols and the grpc port block when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4c5f43c-5a7b-40d6-9e52-721e99a93b1c
📒 Files selected for processing (2)
helm-charts/bifrost/templates/deployment.yamlhelm-charts/bifrost/templates/service.yaml
BearTS
force-pushed
the
05-20-chore_expose_grpc_service_port_if_mentioned
branch
from
36a1311 to
de7044b
Compare
BearTS
force-pushed
the
05-20-chore_add_timeout_example_in_providers_yaml_file
branch
from
f4a7c96 to
0c421c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@helm-charts/bifrost/templates/deployment.yaml`:
- Around line 79-82: The template uses .Values.bifrost.cluster.grpc.port without
ensuring it exists; update the deployment template so that when
.Values.bifrost.cluster.grpc is truthy you require the port value (use Helm's
required function or an explicit check) before rendering the containerPort
stanza; specifically, guard or replace references to
.Values.bifrost.cluster.grpc.port (the grpc port lookup) with a
required("bifrost.cluster.grpc.port is required when bifrost.cluster.grpc is
configured", .Values.bifrost.cluster.grpc.port) call or an if/else that throws a
clear error so Helm fails fast with a meaningful message instead of producing an
invalid manifest.
🪄 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: CHILL
Plan: Pro
Run ID: 2ac42691-fc86-4301-ad34-fd72b5e8915e
📒 Files selected for processing (2)
helm-charts/bifrost/templates/deployment.yamlhelm-charts/bifrost/templates/service.yaml
✅ Files skipped from review due to trivial changes (1)
- helm-charts/bifrost/templates/service.yaml
BearTS
force-pushed
the
05-20-chore_add_timeout_example_in_providers_yaml_file
branch
from
0c421c0 to
d7856f0
Compare
BearTS
force-pushed
the
05-20-chore_expose_grpc_service_port_if_mentioned
branch
from
de7044b to
55c8715
Compare
1 participant
Summary
Adds optional gRPC port support to the Bifrost Helm chart, allowing the cluster to expose a gRPC endpoint when configured.
Changes
TCP) to the deployment template, rendered only whenbifrost.cluster.grpcis defined in valuesgrpcportType of change
Affected areas
How to test
Set the following in your Helm values and confirm the deployment and service include the gRPC port:
helm template ./helm-charts/bifrost | grep -A 4 grpcExpected output should include the
grpcport entry in both the deployment container ports and the service spec.Breaking changes
Related issues
Security considerations
The gRPC port is only exposed when explicitly configured. No authentication or TLS is configured at the Helm chart level; ensure appropriate network policies or ingress controls are in place when enabling this port.
Checklist
docs/contributing/README.mdand followed the guidelines