HYPERFLEET-790 - fix: use kubectl exec instead of kubectl run for maestro consumer creation#27
HYPERFLEET-790 - fix: use kubectl exec instead of kubectl run for maestro consumer creation#27tzhou5 wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Conversation
…stro consumer creation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Makefile's Sequence Diagram(s)sequenceDiagram
participant Make as Makefile target
participant Kubectl as kubectl
participant MaestroPod as deploy/maestro pod
participant MaestroSvc as Maestro HTTP endpoint
Make->>Kubectl: kubectl exec (run curl POST)
Kubectl->>MaestroPod: execute curl inside container
MaestroPod->>MaestroSvc: HTTP POST /consumers
MaestroSvc-->>MaestroPod: 2xx or error response
MaestroPod-->>Kubectl: exit code and output
Kubectl-->>Make: return status
alt failure and attempts < 5
Make->>Make: print attempt failure, sleep 5s, retry
else exhausted attempts
Make-->>Make: print error, exit 1
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
📝 Coding Plan
Comment |
|
/test integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
135-144: Consider handling the "consumer already exists" case for idempotency.If the consumer already exists (e.g., re-running this target), the API likely returns a 4xx error, causing all retries to fail. This breaks re-runnability of the make target.
Additionally,
-s(silent) hides error details, making failures harder to debug. Consider-sSto show errors while suppressing progress output.♻️ Suggested improvement for idempotency and error visibility
`@for` i in 1 2 3 4 5; do \ kubectl exec deploy/maestro --namespace $(MAESTRO_NS) --kubeconfig $(KUBECONFIG) -- \ - curl -sf -X POST \ + curl -sSf -X POST \ -H "Content-Type: application/json" \ http://maestro.$(MAESTRO_NS).svc.cluster.local:8000/api/maestro/v1/consumers \ - -d '{"name": "$(MAESTRO_CONSUMER)"}' && exit 0; \ + -d '{"name": "$(MAESTRO_CONSUMER)"}' && exit 0 || \ + { status=$$?; \ + kubectl exec deploy/maestro --namespace $(MAESTRO_NS) --kubeconfig $(KUBECONFIG) -- \ + curl -sS http://maestro.$(MAESTRO_NS).svc.cluster.local:8000/api/maestro/v1/consumers \ + | grep -q '"name":"$(MAESTRO_CONSUMER)"' && { echo "Consumer already exists"; exit 0; }; \ + }; \ echo " Attempt $$i failed, retrying in 5s..."; \ sleep 5; \ done; \Alternatively, a simpler approach is to check if the consumer exists before attempting creation, or to accept 409 Conflict as success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 135 - 144, The Makefile target that runs kubectl exec deploy/maestro ... curl -sf -X POST to create the Maestro consumer is not idempotent and hides HTTP errors; change the logic so that POST treats a 409/consumer-already-exists response as success (i.e., break/exit 0 on HTTP 201 or 409) or perform a prior GET to confirm existence before POST, and switch curl flags from -s to -sS so error bodies are shown; update the code paths around the curl call and retry loop (the kubectl exec + curl invocation) to implement the 409-acceptance or existence-check behavior and surface errors for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 135-144: The Makefile target that runs kubectl exec deploy/maestro
... curl -sf -X POST to create the Maestro consumer is not idempotent and hides
HTTP errors; change the logic so that POST treats a 409/consumer-already-exists
response as success (i.e., break/exit 0 on HTTP 201 or 409) or perform a prior
GET to confirm existence before POST, and switch curl flags from -s to -sS so
error bodies are shown; update the code paths around the curl call and retry
loop (the kubectl exec + curl invocation) to implement the 409-acceptance or
existence-check behavior and surface errors for debugging.
|
/test integration |
1 participant
Summary by CodeRabbit