Make transport HTTP and HTTPS ports configurable#213
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (5)
📝 WalkthroughWalkthroughThe pull request makes APIM transport listener ports offset-configurable across multiple templates. Hardcoded HTTP (9763) and HTTPS (9443) ports were replaced with Helm-templated expressions that add Sequence Diagram(s)sequenceDiagram
participant Helm as Helm template renderer
participant K8sAPI as Kubernetes API
participant Kubelet as Kubelet
participant APIM as APIM container
Helm->>Helm: compute ports = base + .Values.wso2.apim.portOffset
Helm->>K8sAPI: render & apply Deployment YAML (ports, probes)
K8sAPI->>Kubelet: schedule Pod with rendered spec
Kubelet->>APIM: start container with configured containerPort(s)
APIM->>APIM: bind listeners to computed HTTP/HTTPS ports
Kubelet->>APIM: run startup/liveness/readiness probes against computed ports
Kubelet-->>K8sAPI: report probe results/status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 45 minutes and 25 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
all-in-one/confs/instance-1/deployment.toml (1)
120-126:⚠️ Potential issue | 🟠 MajorKeep the listener ports aligned with the pod spec.
all-in-one/templates/am/instance-1/wso2am-deployment.yamlstill exposes and probes9763/9443, so this change will desynchronize the runtime listener ports from the Kubernetes wiring whenportOffsetis non-zero.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one/confs/instance-1/deployment.toml` around lines 120 - 126, The deployment.toml change adjusts listener ports (via portOffset) but the Kubernetes manifest all-in-one/templates/am/instance-1/wso2am-deployment.yaml still hardcodes and probes ports 9763 and 9443, causing a mismatch; update the Deployment/Service containerPort, readinessProbe, livenessProbe, and Service port definitions in wso2am-deployment.yaml (and any env vars or args referencing 9763/9443) to compute and use the same effective ports as deployment.toml (i.e., add portOffset where used) or derive ports from the same chart values (e.g., use .Values.wso2.apim.configurations.server.portOffset or templated expressions) so the pod spec, probes, and Service remain aligned with the runtime listener ports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@all-in-one/confs/instance-2/deployment.toml`:
- Around line 247-253: The listener ports under the [transport.http] and
[transport.https] blocks use Helm expressions with a portOffset (properties.port
= {{ add 9763 .Values.wso2.apim.portOffset }} and properties.port = {{ add 9443
.Values.wso2.apim.portOffset }}), which can desync from the pod/service/probe
wiring; either remove the offset here and set the concrete pod-facing ports that
the Service/Probe expect, or update the corresponding instance-2
deployment/service/probe manifests to compute their target ports using the same
.Values.wso2.apim.portOffset so the values for properties.port
(transport.http/transport.https) and properties.proxyPort remain aligned. Ensure
the symbols to change are the properties.port lines in [transport.http] and
[transport.https] (or update the deployment/service/probe templates to mirror
the same add ... .Values.wso2.apim.portOffset calculation).
---
Outside diff comments:
In `@all-in-one/confs/instance-1/deployment.toml`:
- Around line 120-126: The deployment.toml change adjusts listener ports (via
portOffset) but the Kubernetes manifest
all-in-one/templates/am/instance-1/wso2am-deployment.yaml still hardcodes and
probes ports 9763 and 9443, causing a mismatch; update the Deployment/Service
containerPort, readinessProbe, livenessProbe, and Service port definitions in
wso2am-deployment.yaml (and any env vars or args referencing 9763/9443) to
compute and use the same effective ports as deployment.toml (i.e., add
portOffset where used) or derive ports from the same chart values (e.g., use
.Values.wso2.apim.configurations.server.portOffset or templated expressions) so
the pod spec, probes, and Service remain aligned with the runtime listener
ports.
🪄 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: 1ec24cd0-933b-4e51-aeff-e78e8f571684
📒 Files selected for processing (3)
all-in-one/confs/instance-1/deployment.tomlall-in-one/confs/instance-2/deployment.tomldistributed/gateway/confs/deployment.toml
1a0ed1e to
df8fe88
Compare
121f3f2 to
97a517a
Compare
Purpose