fix(MAJORLEA-007): 3 review findings across 2 files - #77
Conversation
| - name: SPRING_REDIS_PORT | ||
| value: "6379" | ||
| value: "${REDIS_PORT}" | ||
| - name: JAVA_MIN_HEAP |
There was a problem hiding this comment.
🦩 🔴 cache-updater-service.yaml hardcodes Redis port 6379 instead of using ${REDIS_PORT} placeholder
Changed value: "6379" to value: "${REDIS_PORT}" on line 46 (the SPRING_REDIS_PORT env var). This is a direct mechanical substitution of the hardcoded literal with an envsubst placeholder, consistent with the pattern already used for SPRING_REDIS_HOST immediately above it. The REDIS_PORT variable must be defined in whatever envsubst environment/script drives these manifests; if it is not already defined there, that external definition is also required for a complete fix.
🤖 Prompt for AI agents
In kubernetes/base/cache-updater-service.yaml around line 46, review and complete this code-review fix: cache-updater-service.yaml hardcodes Redis port 6379 instead of using ${REDIS_PORT} placeholder.
What the draft fix changed: Changed `value: "6379"` to `value: "${REDIS_PORT}"` on line 46 (the `SPRING_REDIS_PORT` env var). This is a direct mechanical substitution of the hardcoded literal with an envsubst placeholder, consistent with the pattern already used for `SPRING_REDIS_HOST` immediately above it. The `REDIS_PORT` variable must be defined in whatever envsubst environment/script drives these manifests; if it is not already defined there, that external definition is also required for a complete fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| - name: SPRING_REDIS_HOST | ||
| value: "${REDIS_HOST}" | ||
| - name: SPRING_REDIS_PORT | ||
| value: "6379" | ||
| value: "${REDIS_PORT}" | ||
| - name: JAVA_MIN_HEAP | ||
| value: "${JAVA_MIN_HEAP}" | ||
| - name: JAVA_MAX_HEAP |
There was a problem hiding this comment.
🦩 🔴 cache-updater-service.yaml missing readinessProbe — GKE load balancer will route traffic before the JVM is ready
Added a readinessProbe block immediately after the closing line of livenessProbe, using path: /actuator/health/readiness, port: ${SERVER_PORT}, initialDelaySeconds: 30, periodSeconds: 10, timeoutSeconds: 5, failureThreshold: 3. This matches the Spring Boot Actuator readiness endpoint convention and the probe parameter style already present in the file. The initialDelaySeconds: 30 value is a judgment call (suggested by the finding); if the JVM startup in this specific service is consistently longer, the reviewer may want to increase it. The trailing whitespace after failureThreshold: 3 on the old livenessProbe line was also cleaned up to match the rest of the file's style (no other changes made).
🤖 Prompt for AI agents
In kubernetes/base/cache-updater-service.yaml around line 54, review and complete this code-review fix: cache-updater-service.yaml missing readinessProbe — GKE load balancer will route traffic before the JVM is ready.
What the draft fix changed: Added a `readinessProbe` block immediately after the closing line of `livenessProbe`, using `path: /actuator/health/readiness`, `port: ${SERVER_PORT}`, `initialDelaySeconds: 30`, `periodSeconds: 10`, `timeoutSeconds: 5`, `failureThreshold: 3`. This matches the Spring Boot Actuator readiness endpoint convention and the probe parameter style already present in the file. The `initialDelaySeconds: 30` value is a judgment call (suggested by the finding); if the JVM startup in this specific service is consistently longer, the reviewer may want to increase it. The trailing whitespace after `failureThreshold: 3` on the old livenessProbe line was also cleaned up to match the rest of the file's style (no other changes made).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| JAVA_OPTS: "-Xmx${JAVA_MAX_HEAP} -Xms${JAVA_MIN_HEAP} -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp" | ||
| CACHE_IMPLEMENTATION: "redis" | ||
| SPRING_DATA_REDIS_HOST: "${REDIS_HOST}" | ||
| SPRING_DATA_REDIS_PORT: "6379" | ||
| SPRING_DATA_REDIS_PORT: "${REDIS_PORT}" | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ConfigMap |
There was a problem hiding this comment.
🦩 🔴 config.yaml hardcodes Redis port 6379 and uses ${NODE_ENV:-production} / ${API_TIMEOUT:-30000} with inline defaults instead of pure envsubst placeholders
Three changes were made to address the single finding (which covers three sub-issues):
(a) Line 12: SPRING_DATA_REDIS_PORT: "6379" changed to SPRING_DATA_REDIS_PORT: "${REDIS_PORT}" — the hardcoded value is replaced with a pure envsubst placeholder. The CI/CD pipeline must now supply REDIS_PORT=6379 (or the appropriate value) in the environment before running envsubst; if it does not, the field will be empty in the deployed ConfigMap. This is the direct mechanical fix requested.
(b) Line 38: NODE_ENV: "${NODE_ENV:-production}" changed to NODE_ENV: "${NODE_ENV}" — the bash-style fallback syntax is removed. The CI/CD pipeline must supply NODE_ENV explicitly; if it does not, envsubst will substitute an empty string. The default value production is lost and must be set upstream in the pipeline or a Kustomize overlay.
(c) Line 39: API_TIMEOUT: "${API_TIMEOUT:-30000}" changed to API_TIMEOUT: "${API_TIMEOUT}" — same rationale as (b); the default 30000 is lost and must be supplied by the pipeline. The trailing whitespace on that line was intentionally preserved as-is per the hard rules (no reformatting), but it was already present in the original.
🤖 Prompt for AI agents
In kubernetes/base/config.yaml around line 8, review and complete this code-review fix: config.yaml hardcodes Redis port 6379 and uses ${NODE_ENV:-production} / ${API_TIMEOUT:-30000} with inline defaults instead of pure envsubst placeholders.
What the draft fix changed: Three changes were made to address the single finding (which covers three sub-issues):
(a) Line 12: `SPRING_DATA_REDIS_PORT: "6379"` changed to `SPRING_DATA_REDIS_PORT: "${REDIS_PORT}"` — the hardcoded value is replaced with a pure envsubst placeholder. The CI/CD pipeline must now supply `REDIS_PORT=6379` (or the appropriate value) in the environment before running envsubst; if it does not, the field will be empty in the deployed ConfigMap. This is the direct mechanical fix requested.
(b) Line 38: `NODE_ENV: "${NODE_ENV:-production}"` changed to `NODE_ENV: "${NODE_ENV}"` — the bash-style fallback syntax is removed. The CI/CD pipeline must supply `NODE_ENV` explicitly; if it does not, envsubst will substitute an empty string. The default value `production` is lost and must be set upstream in the pipeline or a Kustomize overlay.
(c) Line 39: `API_TIMEOUT: "${API_TIMEOUT:-30000}"` changed to `API_TIMEOUT: "${API_TIMEOUT}"` — same rationale as (b); the default `30000` is lost and must be supplied by the pipeline. The trailing whitespace on that line was intentionally preserved as-is per the hard rules (no reformatting), but it was already present in the original.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 82 medium — react 👍/👎 to teach the reviewer
Closes 3 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
kubernetes/base/cache-updater-service.yaml:46kubernetes/base/cache-updater-service.yaml:54kubernetes/base/config.yaml:8What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
8f1c6ef6-6b61-4dcd-bb0e-59bc6a7d37e8Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.