Ci monitor consolidation - #245
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vimauro The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughChangesDoctor fragment dashboard integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.23.1)payload-monitor/README.mdmarkdownlint-cli2 v0.23.1 (markdownlint v0.41.1) ... [truncated 1137 characters] ... node:internal/modules/esm/resolve:271:11) plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.mdmarkdownlint-cli2 v0.23.1 (markdownlint v0.41.1) ... [truncated 1137 characters] ... node:internal/modules/esm/resolve:271:11) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
payload-monitor/payload_monitor/report/templates/styles.css (1)
1235-1285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeprecated
word-break: break-wordflagged by stylelint.
overflow-wrap: anywhereon the same rule already provides the intended wrapping;word-break: break-wordis a deprecated value for this purpose.♻️ Proposed fix
-.doctor-detail-row td { padding: 0 6px 12px 40px; overflow-wrap: anywhere; word-break: break-word; } +.doctor-detail-row td { padding: 0 6px 12px 40px; overflow-wrap: anywhere; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-monitor/payload_monitor/report/templates/styles.css` around lines 1235 - 1285, Remove the deprecated word-break: break-word declaration from .doctor-detail-row td. Keep overflow-wrap: anywhere unchanged so long detail content continues wrapping correctly.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md`:
- Around line 271-277: The Error Handling section in the dashboard workflow
should not remain as a trailing consolidated block. Move the prepare failure
rule next to the 3b.1 prepare commands, remove the duplicate analysis failure
rule because 3b.5 already mandates continuation, move the finalize failure
behavior next to 3b.6’s FileNotFoundError handling, and place the doctor-tab
count behavior beside 3b.6’s closing note; then remove the obsolete trailing
section.
In `@plugins/shared/scripts/create-report.py`:
- Line 1098: Update the hyphen separators in the report-rendering paths around
the evidence output and the referenced TOC/status locations to include a
trailing space before the following text. Apply the same spacing consistently to
the code handling `item`, lines 1631-1634, 1809, and 1820-1826, while preserving
the existing HTML escaping and rendering behavior.
- Around line 1998-2011: Update _doctor_prefix_ids to also prefix PR anchor ids
and matching href targets beginning with “pr-” using the provided slug,
alongside the existing release and images handling. Ensure both base PR ids and
issue-specific PR ids emitted by render_pr_section are rewritten consistently so
internal anchor navigation remains valid.
---
Nitpick comments:
In `@payload-monitor/payload_monitor/report/templates/styles.css`:
- Around line 1235-1285: Remove the deprecated word-break: break-word
declaration from .doctor-detail-row td. Keep overflow-wrap: anywhere unchanged
so long detail content continues wrapping correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 11f1c37a-45b6-452a-9ac5-254e29013324
📒 Files selected for processing (10)
payload-monitor/README.mdpayload-monitor/payload_monitor/models.pypayload-monitor/payload_monitor/report/generator.pypayload-monitor/payload_monitor/report/templates/dashboard.htmlpayload-monitor/payload_monitor/report/templates/scripts.jspayload-monitor/payload_monitor/report/templates/styles.cssplugins/edge-ocp-ci/skills/generate-dashboard/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.sh
| #### Error Handling | ||
|
|
||
| - If prepare fails for a component, skip all subsequent steps for that component | ||
| - If analysis agents fail or time out, proceed to finalize - reports will note missing analysis | ||
| - If finalize fails for a component, its fragment won't exist and is simply not injected | ||
| - The dashboard works with zero, one, or any number of doctor tabs | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Error Handling rules should be co-located with their steps, not trailing at the end of 3b.
This section bundles failure policy for three different steps (3b.1 prepare, 3b.3 analysis, 3b.5 finalize) into one block after 3b.6. Based on learnings, failure policies/edge-case rules should be placed inline right next to the step they apply to (using "CRITICAL" for hard stops and "Error Handling" bullets for partial failures directly in the step context), since an LLM agent executing linearly won't reliably re-consult a separate trailing section — this creates two sources of truth without improving compliance.
Suggested placement:
- "If prepare fails for a component, skip all subsequent steps for that component" → move next to the 3b.1
doctor.sh preparecommands (near line 126, alongside the existing "If a release haserrorin its entry... skip it" note). - "If analysis agents fail or time out, proceed to finalize" → already effectively covered by the 3b.5 "IMPORTANT: MANDATORY" note; consider removing the duplicate here.
- "If finalize fails for a component, its fragment won't exist and is simply not injected" → move near 3b.6's
try/except FileNotFoundErrorblock (line 254), which already implements this behavior. - "The dashboard works with zero, one, or any number of doctor tabs" → fits naturally next to the closing note in 3b.6 ("Doctor tabs appear automatically...").
Based on learnings, "co-locate failure policies and edge-case rules inline with the specific step that needs them... Do not move these rules to a separate top-level 'Edge Cases' (or similar) section."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md` around lines 271 -
277, The Error Handling section in the dashboard workflow should not remain as a
trailing consolidated block. Move the prepare failure rule next to the 3b.1
prepare commands, remove the duplicate analysis failure rule because 3b.5
already mandates continuation, move the finalize failure behavior next to 3b.6’s
FileNotFoundError handling, and place the doctor-tab count behavior beside
3b.6’s closing note; then remove the obsolete trailing section.
Source: Learnings
| item = _e(link.get("cause")) | ||
| if link.get("evidence"): | ||
| item += f' — <span class="evidence">{_e(link["evidence"])}</span>' | ||
| item += f' -<span class="evidence">{_e(link["evidence"])}</span>' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing space after hyphen produces squished TOC/evidence text.
Several —→hyphen replacements dropped the trailing space, so rendered text reads like "-collection error", "-3 failures", "-5 running" (hyphen mashed against the next word/number, easily misread as a negative number).
🐛 Proposed fix
- item += f' -<span class="evidence">{_e(link["evidence"])}</span>'
+ item += f' - <span class="evidence">{_e(link["evidence"])}</span>'- suffix = f' -{pending} running' if pending else ''
+ suffix = f' - {pending} running' if pending else ''
toc_lines.append(
f' <li><a href="`#pr-`{pr["number"]}">PR# {pr["number"]}</a>'
- f' -{pr["failed"]} failures ({b.get("build", 0)} build, {b.get("test", 0)} test, {b.get("infrastructure", 0)} infra){suffix}</li>'
+ f' - {pr["failed"]} failures ({b.get("build", 0)} build, {b.get("test", 0)} test, {b.get("infrastructure", 0)} infra){suffix}</li>'
) toc.append(
- f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> -collection error</li>'
+ f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> - collection error</li>'
) toc.append(
- f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> -'
+ f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> - '
f'<span class="toc-counts" data-release="{_e(version)}">'
f'{rdata["total_failed"]} failures ({b["build"]} build, {b["test"]} test, {b["infrastructure"]} infra)'
f'{pass_info}</span></li>'
)
else:
- toc.append(f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> -no data</li>')
+ toc.append(f' <li><a href="`#release-`{_e(version)}">Release {_e(version)}</a> - no data</li>')Also applies to: 1631-1634, 1809-1809, 1820-1826
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/shared/scripts/create-report.py` at line 1098, Update the hyphen
separators in the report-rendering paths around the evidence output and the
referenced TOC/status locations to include a trailing space before the following
text. Apply the same spacing consistently to the code handling `item`, lines
1631-1634, 1809, and 1820-1826, while preserving the existing HTML escaping and
rendering behavior.
| def _doctor_prefix_ids(html, slug): | ||
| """Prefix id attributes to avoid collisions with payload-monitor.""" | ||
| html = re.sub(r'id="release-', f'id="doctor-{slug}-release-', html) | ||
| html = re.sub(r'href="#release-', f'href="#doctor-{slug}-release-', html) | ||
| html = re.sub(r'id="images-', f'id="doctor-{slug}-images-', html) | ||
| html = re.sub(r'href="#images-', f'href="#doctor-{slug}-images-', html) | ||
| return html | ||
|
|
||
|
|
||
| def _postprocess_fragment(html, slug): | ||
| """Prefix CSS classes and HTML ids for embedding in the payload-monitor dashboard.""" | ||
| html = _doctor_prefix_classes(html) | ||
| return _doctor_prefix_ids(html, slug) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_doctor_prefix_ids doesn't cover PR anchor ids, risking id collisions between embedded doctor fragments.
The function only rewrites id="release-…" and id="images-…" (plus their href="#…" counterparts), but render_pr_section (used by generate_fragment) emits unprefixed id="pr-{number}" and id="pr-{number}-{issue}"/href="#pr-…" anchors. When multiple doctor fragments (e.g. future components that both track rebase PRs) are injected into the same payload-monitor dashboard, these ids collide, breaking href="#pr-…" anchor navigation (the browser jumps to whichever element appears first in the DOM) and producing invalid duplicate-id HTML — undermining the function's own stated purpose of avoiding collisions.
🐛 Proposed fix
def _doctor_prefix_ids(html, slug):
"""Prefix id attributes to avoid collisions with payload-monitor."""
html = re.sub(r'id="release-', f'id="doctor-{slug}-release-', html)
html = re.sub(r'href="`#release-`', f'href="`#doctor-`{slug}-release-', html)
html = re.sub(r'id="images-', f'id="doctor-{slug}-images-', html)
html = re.sub(r'href="`#images-`', f'href="`#doctor-`{slug}-images-', html)
+ html = re.sub(r'id="pr-', f'id="doctor-{slug}-pr-', html)
+ html = re.sub(r'href="`#pr-`', f'href="`#doctor-`{slug}-pr-', html)
return html📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _doctor_prefix_ids(html, slug): | |
| """Prefix id attributes to avoid collisions with payload-monitor.""" | |
| html = re.sub(r'id="release-', f'id="doctor-{slug}-release-', html) | |
| html = re.sub(r'href="#release-', f'href="#doctor-{slug}-release-', html) | |
| html = re.sub(r'id="images-', f'id="doctor-{slug}-images-', html) | |
| html = re.sub(r'href="#images-', f'href="#doctor-{slug}-images-', html) | |
| return html | |
| def _postprocess_fragment(html, slug): | |
| """Prefix CSS classes and HTML ids for embedding in the payload-monitor dashboard.""" | |
| html = _doctor_prefix_classes(html) | |
| return _doctor_prefix_ids(html, slug) | |
| def _doctor_prefix_ids(html, slug): | |
| """Prefix id attributes to avoid collisions with payload-monitor.""" | |
| html = re.sub(r'id="release-', f'id="doctor-{slug}-release-', html) | |
| html = re.sub(r'href="`#release-`', f'href="`#doctor-`{slug}-release-', html) | |
| html = re.sub(r'id="images-', f'id="doctor-{slug}-images-', html) | |
| html = re.sub(r'href="`#images-`', f'href="`#doctor-`{slug}-images-', html) | |
| html = re.sub(r'id="pr-', f'id="doctor-{slug}-pr-', html) | |
| html = re.sub(r'href="`#pr-`', f'href="`#doctor-`{slug}-pr-', html) | |
| return html | |
| def _postprocess_fragment(html, slug): | |
| """Prefix CSS classes and HTML ids for embedding in the payload-monitor dashboard.""" | |
| html = _doctor_prefix_classes(html) | |
| return _doctor_prefix_ids(html, slug) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/shared/scripts/create-report.py` around lines 1998 - 2011, Update
_doctor_prefix_ids to also prefix PR anchor ids and matching href targets
beginning with “pr-” using the provided slug, alongside the existing release and
images handling. Ensure both base PR ids and issue-specific PR ids emitted by
render_pr_section are rewritten consistently so internal anchor navigation
remains valid.
| ```bash | ||
| bash "$SHARED_SCRIPTS/doctor.sh" prepare \ | ||
| --component microshift --workdir "$MICROSHIFT_WORKDIR" \ | ||
| --rebase --repo openshift/microshift <VERSIONS> |
There was a problem hiding this comment.
doctor.sh prepare doesn't accept --rebase (only --pull-requests/--repo, doctor.sh:49), so it hits the -*) catch-all and return 1s under set -euo pipefail, aborting the whole MicroShift prepare phase (reproduced live).
| performance graphs when relevant. | ||
| 3. After the analysis completes, save the FULL report output (including the | ||
| --- STRUCTURED SUMMARY --- block) to: | ||
| <MICROSHIFT_WORKDIR>/jobs/release-<RELEASE>-job-<N>-<BUILD_ID>.txt |
There was a problem hiding this comment.
Per-job reports are saved as .txt but aggregate.py only globs *.json (lines 140/145), so all analysis is silently dropped — restore .json here (as the standalone skills do) on lines 170, 186, and 197.
| print("No valid job reports found", file=sys.stderr) | ||
| sys.exit(1) | ||
| result = build_release_json(release, [], timestamp) | ||
| result["no_job_files"] = True |
There was a problem hiding this comment.
no_job_files is set whenever zero per-job files exist and is rendered as green "all clear" (create-report.py:1417-1419), producing a false pass for any release that had failures but no analysis (guaranteed by the .txt/.json mismatch, or when agents time out).
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation