Skip to content

Conversation

@rsoaresd
Copy link
Contributor

@rsoaresd rsoaresd commented Dec 18, 2025

Description

When running the e2e tests in devsandbox-dashboard, we are calling prepare-and-deploy-e2e test-devsandbox-dashboard-e2e . I did not notice locally, but on the pr to onboard the repo in CI sometimes it is flaky because the route of registration-service is not yet ready by the time we access https://github.com/codeready-toolchain/toolchain-e2e/blob/master/scripts/ci/manage-devsandbox-dashboard.sh#L180

So, this PR waits for it

Summary by CodeRabbit

  • Chores
    • Enhanced deployment pipeline with improved synchronization checks to ensure registration service readiness before proceeding.
    • Added timeout-based route polling with progress indicators for better deployment visibility.
    • Strengthened error handling with detailed diagnostics when services fail to become available within expected timeframes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

A bash script update to the DevSandbox CI deployment process adds synchronization waits in the DEPLOY_UI path. The script now waits for the registration-service deployment to become available and polls for the registration-service route with timeout logic before constructing API URLs.

Changes

Cohort / File(s) Summary
Deployment synchronization waits
scripts/ci/manage-devsandbox-dashboard.sh
Added blocking waits for registration-service deployment availability and route creation; implemented timeout-based polling with progress indicator; error handling that lists routes on failure; reordered REGISTRATION_SERVICE_API URL construction to depend on route availability

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Single file change confined to shell script logic
  • Timeout and polling logic implementation should be verified for correctness
  • Error handling and exit conditions merit attention

Suggested labels

approved

Suggested reviewers

  • metlos
  • alexeykazakov
  • fbm3307
  • xcoulon

Poem

🐰 A dash script now learns patience true,
Waiting for routes to peek on through,
With timeouts and polls, it checks with care,
Before URLs dance in the DevSandbox air! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a wait for the registration-service route in the manage-devsandbox-dashboard.sh script.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)

180-193: Consider aligning route wait timeout with deployment timeout.

The deployment wait uses a 5-minute timeout, but the route wait uses an effective timeout of ~2 minutes (60 iterations × 2 seconds). While routes typically become available quickly, aligning both timeouts would provide consistency and additional buffer if the deployment takes most of the 5 minutes.

🔎 Consider this adjustment for timeout consistency:
-    NEXT_WAIT_TIME=0
-    while [[ -z $(oc get routes registration-service -n ${HOST_NS} -o jsonpath='{.status.ingress[0].host}' 2>/dev/null || true) ]]; do
-        if [[ ${NEXT_WAIT_TIME} -eq 60 ]]; then
+    TIMEOUT_SECONDS=300  # 5 minutes to match deployment timeout
+    NEXT_WAIT_TIME=0
+    while [[ -z $(oc get routes registration-service -n ${HOST_NS} -o jsonpath='{.status.ingress[0].host}' 2>/dev/null || true) ]]; do
+        if [[ ${NEXT_WAIT_TIME} -ge ${TIMEOUT_SECONDS} ]]; then
             echo ""
             echo "[$(date '+%Y-%m-%d %H:%M:%S')] ERROR: Timeout waiting for registration-service route"
             oc get routes -n ${HOST_NS}
             exit 1
         fi
         echo -n "."
-        sleep 2
+        sleep 1
         ((NEXT_WAIT_TIME++))
     done
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f4432e and 210c8af.

📒 Files selected for processing (1)
  • scripts/ci/manage-devsandbox-dashboard.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.

Applied to files:

  • scripts/ci/manage-devsandbox-dashboard.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Unit Tests
  • GitHub Check: GolangCI Lint
  • GitHub Check: Build & push operator bundles & dashboard image for e2e tests

Comment on lines +179 to +194
# Wait for registration-service deployment and route to be ready
oc wait --for=condition=Available deployment/registration-service -n ${HOST_NS} --timeout=5m
# Wait for registration-service route to exist
NEXT_WAIT_TIME=0
while [[ -z $(oc get routes registration-service -n ${HOST_NS} -o jsonpath='{.status.ingress[0].host}' 2>/dev/null || true) ]]; do
if [[ ${NEXT_WAIT_TIME} -eq 60 ]]; then
echo ""
echo "[$(date '+%Y-%m-%d %H:%M:%S')] ERROR: Timeout waiting for registration-service route"
oc get routes -n ${HOST_NS}
exit 1
fi
echo -n "."
sleep 2
((NEXT_WAIT_TIME++))
done

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add newline after successful route wait.

The progress indicator prints dots during the wait (line 190), but when the loop exits successfully, there's no newline or success message. This causes the dots to run into the next output line, making the logs harder to read.

🔎 Apply this diff to improve output formatting:
     done
+    echo ""
+    echo "[$(date '+%Y-%m-%d %H:%M:%S')] registration-service route is ready"
 
     # Get the Registration Service API URL
🤖 Prompt for AI Agents
In scripts/ci/manage-devsandbox-dashboard.sh around lines 179 to 194, the
progress dots printed while waiting for the registration-service route do not
end with a newline or success message, causing the dots to run into subsequent
log output; after the waiting loop completes successfully, add a newline (e.g.,
echo "" or printf "\n") and optionally a short success/log message (with
timestamp) so the dots end on their own line and the logs remain readable.

Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@openshift-ci
Copy link

openshift-ci bot commented Dec 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, rsoaresd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [MatousJobanek,alexeykazakov,mfrancisc,rsoaresd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rsoaresd rsoaresd merged commit c4598f6 into codeready-toolchain:master Dec 19, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants