Skip to content

fix(MAJORLEA-007): 3 review findings across 2 files #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 2 commits into
base: main
Choose a base branch
from
+14 −6
Draft

fix(MAJORLEA-007): 3 review findings across 2 files #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5785768
fix(MAJORLEA-007): 3 review findings across 2 files
flamingo[bot] Aug 14, 2026
8ba36e9
fix(MAJORLEA-007): 3 review findings across 2 files
flamingo[bot] Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions kubernetes/base/cache-updater-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ spec:
- name: SPRING_REDIS_HOST
value: "${REDIS_HOST}"
- name: SPRING_REDIS_PORT
value: "6379"
value: "${REDIS_PORT}"
- name: JAVA_MIN_HEAP

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

value: "${JAVA_MIN_HEAP}"
- name: JAVA_MAX_HEAP
Comment on lines 42 to 48

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand All @@ -69,7 +69,15 @@ spec:
initialDelaySeconds: 90
periodSeconds: 30
timeoutSeconds: 5
failureThreshold: 3
failureThreshold: 3
readinessProbe:
httpGet:
path: /actuator/health/readiness
port: ${SERVER_PORT}
initialDelaySeconds: 30
periodSeconds: 10
timeoutSeconds: 5
failureThreshold: 3
---
apiVersion: v1
kind: Service
Expand All @@ -86,4 +94,4 @@ spec:
- name: http
port: ${SERVER_PORT}
targetPort: ${SERVER_PORT}
type: ClusterIP
type: ClusterIP
6 changes: 3 additions & 3 deletions kubernetes/base/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ data:
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
Comment on lines 9 to 15

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down Expand Up @@ -42,5 +42,5 @@ kind: ConfigMap
metadata:
name: frontend-config
data:
NODE_ENV: "${NODE_ENV:-production}"
API_TIMEOUT: "${API_TIMEOUT:-30000}"
NODE_ENV: "${NODE_ENV}"
API_TIMEOUT: "${API_TIMEOUT}"