Skip to content

Conversation

@elfiesmelfie
Copy link

@elfiesmelfie elfiesmelfie commented Dec 3, 2025

Simplifies the health check messages in wait_cluster_become_healthy.yaml
by using a single assert task instead of separate conditional debug
and fail tasks. This is more idiomatic Ansible and reduces duplication.

This additionally addresses the unconditional debug message that was
indiacting that the check were successful in all circumstances.

Summary by CodeRabbit

  • Bug Fixes
    • Consolidated health check so success confirmations only appear when all components are healthy, preventing false-positive deployment messages.
    • Unified success/failure messaging for clearer, single-path handling of cluster health outcomes.

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

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Walkthrough

Replaces two separate tasks (debug success message and conditional fail) with a single Ansible assert task in the cluster health-check play. The assertion uses the same condition ("'False' not in component_status.stdout_lines") and includes both fail_msg and success_msg.

Changes

Cohort / File(s) Summary
Cluster health assertion
ansible/roles/deploy-crc-cloud/tasks/wait_cluster_become_healthy.yaml
Replaced the previous two-task pattern (a debug success message task guarded by a conditional and a separate fail task) with a single assert task that evaluates "'False' not in component_status.stdout_lines" and provides fail_msg and success_msg.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review focus:
    • Confirm component_status is always defined/populated before the assert.
    • Validate the assertion condition handles all expected output shapes (e.g., empty, multiline).
    • Ensure message wording and failure semantics align with play expectations.

Poem

🐰 I nibble on YAML, tidy and neat,
One assert to check every heartbeat,
If no "False" is found I hop with delight,
Else I thump my foot through the quiet night. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing debug and fail tasks with a single assert task in the wait_cluster_become_healthy.yaml file.
✨ 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.

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49d5584 and 2c4ca43.

📒 Files selected for processing (1)
  • ansible/roles/deploy-crc-cloud/tasks/wait_cluster_become_healthy.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
ansible/roles/deploy-crc-cloud/tasks/wait_cluster_become_healthy.yaml

[error] 16-16: syntax error: expected , but found ''

(syntax)

🔇 Additional comments (1)
ansible/roles/deploy-crc-cloud/tasks/wait_cluster_become_healthy.yaml (1)

15-18: Condition logic is sound.

Once the syntax error is corrected, the conditional guard correctly ensures the success message only displays when all components are healthy (no 'False' values in the output), aligning with the PR objectives.

Simplifies the health check messages in wait_cluster_become_healthy.yaml
by using a single assert task instead of separate conditional debug
and fail tasks. This is more idiomatic Ansible and reduces duplication.

This additionally addresses the unconditional debug message that was
indiacting that the check were successful in all circumstances.

Assisted-by: Claude
@elfiesmelfie elfiesmelfie changed the title [roles/deploy-crc-cloud] Add condition for outputting success [roles/deploy-crc-cloud] Replace debug and fail tasks with assert Dec 3, 2025
@danpawlik
Copy link
Collaborator

/lgtm

@elfiesmelfie
Copy link
Author

recheck

Copy link
Collaborator

@danpawlik danpawlik left a comment

Choose a reason for hiding this comment

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

In the end, it finish with an error.
EDIT:
checking again.

EDIT:
need to check, why now it stuck on place where many pods are in "Pending" state.
In the end, solution to make it running is:

oc adm certificate approve $(oc get csr --no-headers | awk '/Pending/ {print $1}')

@danpawlik
Copy link
Collaborator

@elfiesmelfie Let's wait for crc-cloud team to merge this change - #211 then I would test your change again.
Thanks for contribution!

Copy link
Collaborator

@danpawlik danpawlik left a comment

Choose a reason for hiding this comment

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

Waiting for merge #211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants