OCPEDGE-2685: Refactor CI doctor scripts into shared plugin infrastructure#145
Conversation
openshift-ci
Bot
added
the
approved
WalkthroughConsolidates microshift-ci CI tooling into a shared scripts layer: adds full-featured shared scripts for discovery, downloading, aggregation, classification, report generation, graphing, and orchestration; replaces plugin-level implementations with one-line delegating wrappers; and updates microshift SKILL docs to pass ChangesShared CI workflow and plugin delegation
Suggested labels: ✨ Finishing Touches🧪 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.22.1)plugins/microshift-ci/skills/continue-session/SKILL.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) ... [truncated 1102 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/skills/doctor-refresh/SKILL.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) ... [truncated 1100 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/skills/doctor/SKILL.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) ... [truncated 1092 characters] ... node:internal/modules/esm/resolve:271:11) 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. 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: 7
🤖 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/shared/scripts/aggregate.py`:
- Around line 122-135: The helper _normalize_step_name currently hardcodes
"openshift-microshift-" which breaks deterministic bucketing for other products;
change it to use a configurable prefix (e.g., a module-level _STEP_PREFIX) and
expose wiring so callers set it from CLI or env (mirror
create-report.py/doctor.sh behavior): add a --product argument in main() that
sets _STEP_PREFIX = f"openshift-{product}-" (or read PRODUCT_PREFIX env var),
and update _normalize_step_name to search for rf"({_STEP_PREFIX}\S+)" so
group_by_signature and other consumers get normalized names for any product.
In `@plugins/shared/scripts/create-report.py`:
- Line 19: The script currently constructs a default workdir using a hardcoded
"/tmp"; replace that with a secure, ruff-compliant temporary directory creation
(e.g., use tempfile.mkdtemp() or tempfile.TemporaryDirectory() and/or
Path(tempfile.gettempdir()) combined with pathlib.Path) and make the default
overridable via an environment variable or function parameter; add the required
imports (tempfile, pathlib or os as needed), update any references to the
previous "/tmp" constant (e.g., the variable or parameter named default_workdir
or similar in the create-report logic) to use the new tempfile-based value, and
ensure the code passes ruff checks.
- Around line 1161-1189: The script currently exits whenever no summaries/status
are present (using found_any) even though artifact collections exist (releases,
prs, open_bugs), causing premature aborts for jobs-only or error-only runs;
update the logic that decides to exit so it checks for the presence of any
artifact collections rather than only summaries: compute a boolean (e.g.,
has_artifacts) by checking files["releases"] non-empty, files["prs"] or
pr_entry["bugs"] non-empty or files["open_bugs"] present (and/or
pr_entry["status"]/summary as additional info), and use that instead of
found_any in the final if before sys.exit(1) so the script only errors when
truly no analysis files exist in workdir. Ensure you reference the same
variables (files, releases, pr_entry, found_any, workdir) when making the
change.
- Around line 638-639: The blind "except Exception as e" in the PNG processing
block should be replaced with explicit exception types that the operation can
raise (e.g., OSError, ValueError, or PIL.UnidentifiedImageError if using Pillow)
and re-raise any unexpected exceptions; update the except clause that references
the variable "png" to catch those specific exceptions and keep the same warning
print for handled cases while allowing other errors to propagate.
In `@plugins/shared/scripts/doctor.sh`:
- Around line 123-124: The call to prow-jobs-for-pull-requests.sh currently
hardcodes the author "microshift-rebase-script[bot]"; change this to use a
configurable variable (e.g., REBASE_BOT) and make the script accept a new flag
--rebase-bot (or derive REBASE_BOT from PRODUCT with a default of
"${PRODUCT}-rebase-script[bot]") so the call becomes --author "${REBASE_BOT}";
update the flag parsing logic in doctor.sh to populate REBASE_BOT (falling back
to the PRODUCT-based convention) and use that variable in the pr_json assignment
that invokes prow-jobs-for-pull-requests.sh.
- Line 302: The WORKDIR assignment uses a hardcoded fallback
(${PRODUCT:-microshift}) which causes graphs to be written to a microshift
directory whenever PRODUCT is unset; change this so PRODUCT must be provided:
remove the "microshift" default from the WORKDIR construction (use ${PRODUCT} or
expand only when set) and add a validation check before WORKDIR is computed
(e.g., in the same function or in cmd_prepare/cmd_finalize) that exits with a
clear error if PRODUCT is empty; alternatively, make the graphs command require
--product in its argument parsing and error out if not supplied so WORKDIR
cannot be constructed without an explicit PRODUCT.
In `@plugins/shared/scripts/download-jobs.sh`:
- Around line 167-169: The loop over failed_ids uses an unquoted expansion
(failed_ids) causing SC2086; replace the for-loop with a safe while-read pattern
that preserves line boundaries and avoids word-splitting: feed failed_ids into a
here-string or echo piped into "while IFS= read -r bid; do" and inside the loop
update output_json with the existing jq command using the quoted variable ($bid)
as before, ensuring all variable expansions are quoted (failed_ids is not
expanded directly in the loop header and bid is read with read -r) so the script
is shellcheck-clean.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ce9c2a60-0426-4d69-b041-558e488ba137
📒 Files selected for processing (18)
plugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/doctor-refresh/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/download-jobs.shplugins/shared/scripts/prow-jobs-for-release.sh
kasturinarra
force-pushed
the
lvms_ci_doctor_p1
branch
from
cf13426 to
b354a3e
Compare
kasturinarra
force-pushed
the
lvms_ci_doctor_p1
branch
from
b354a3e to
56585e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/shared/scripts/create-report.py (1)
985-985: 💤 Low valueDrop the product-specific default for
product_titlein a shared helper.Now that this module lives under
plugins/shared/, defaultingproduct_title="MicroShift"ingenerate_htmlleaks a product identity into a generic helper.main()already always suppliesproduct_title, so the default never fires in practice; making it a required keyword argument (or defaulting to an obviously generic placeholder) prevents a future caller from silently rendering a "MicroShift" header for, e.g., LVMS.♻️ Suggested change
-def generate_html(releases_data, all_bug_candidates, pr_data, pr_status, timestamp, pr_error=None, bugs_tab_data=None, product_title="MicroShift"): +def generate_html(releases_data, all_bug_candidates, pr_data, pr_status, timestamp, pr_error=None, bugs_tab_data=None, *, product_title):🤖 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 985, The generate_html function currently defaults product_title="MicroShift", which leaks a product identity from a shared helper; change the signature for generate_html to require product_title as a keyword argument (remove the "MicroShift" default) or replace it with a neutral placeholder (e.g., None) and assert/raise if not provided; update callers if any (note main() already passes product_title) and ensure the function uses the provided product_title value (refer to generate_html to locate the change).
🤖 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.
Nitpick comments:
In `@plugins/shared/scripts/create-report.py`:
- Line 985: The generate_html function currently defaults
product_title="MicroShift", which leaks a product identity from a shared helper;
change the signature for generate_html to require product_title as a keyword
argument (remove the "MicroShift" default) or replace it with a neutral
placeholder (e.g., None) and assert/raise if not provided; update callers if any
(note main() already passes product_title) and ensure the function uses the
provided product_title value (refer to generate_html to locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 0438f56a-334f-474f-87cf-d3e533cb7ea4
📒 Files selected for processing (18)
plugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/doctor-refresh/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/download-jobs.shplugins/shared/scripts/prow-jobs-for-release.sh
✅ Files skipped from review due to trivial changes (6)
- plugins/microshift-ci/scripts/doctor.sh
- plugins/microshift-ci/scripts/create-report.py
- plugins/microshift-ci/skills/continue-session/SKILL.md
- plugins/microshift-ci/scripts/prow-jobs-for-release.sh
- plugins/microshift-ci/skills/doctor/SKILL.md
- plugins/microshift-ci/scripts/aggregate.py
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/microshift-ci/skills/doctor-refresh/SKILL.md
- plugins/microshift-ci/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/doctor.sh
- plugins/shared/scripts/prow-jobs-for-release.sh
- plugins/shared/scripts/download-jobs.sh
- plugins/shared/scripts/doctor.sh
kasturinarra
force-pushed
the
lvms_ci_doctor_p1
branch
from
56585e3 to
6cb8b7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
plugins/shared/scripts/prow-jobs-for-release.sh (1)
30-59: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFlag naming is inconsistent with the rest of the shared CLI surface (
--product).The sibling shared scripts being introduced in this PR standardize on
--product(doctor.sh,create-report.py), and the SKILL.md updates in the PR description pass--product microshift. This script exposes the same concept as--component, which:
- Diverges from the consistent CLI surface the refactor is meant to establish.
- Mismatches the PR description, which lists "added
--filterparameter" for this script — neither--filternor--productactually appears here.- Re-surfaces a concern previously raised on this script ("Should it be
--productinstead?") that does not appear to have been resolved.Recommend renaming
--component/componentto--product/producthere (and in the matching forwarder inplugins/microshift-ci/scripts/) so all shared scripts share one vocabulary.♻️ Proposed rename
-# Fetch all jobs matching a component for a release, return latest run per job as JSON +# Fetch all jobs matching a product for a release, return latest run per job as JSON fetch_latest_per_job() { local release="${1}" - local component="${2}" - curl -s --max-time 300 --retry 3 --retry-delay 5 --compressed "${PROW_URL}" | jq --arg release "${release}" --arg component "${component}" ' - [.[] | select((.job | contains($component)) and (.job | contains($release)))] | + local product="${2}" + curl -s --max-time 300 --retry 3 --retry-delay 5 --compressed "${PROW_URL}" | jq --arg release "${release}" --arg product "${product}" ' + [.[] | select((.job | contains($product)) and (.job | contains($release)))] | @@ usage() { - echo "Usage: ${0} [--mode MODE] --component <component> <release>" >&2 + echo "Usage: ${0} [--mode MODE] --product <product> <release>" >&2 echo " --mode MODE: Operation mode (default: failed)" >&2 echo " status: Latest run status for each job" >&2 echo " failed: Only jobs with failure status" >&2 - echo " --component C: Component name used to filter jobs (e.g., microshift, lvm-operator)" >&2 + echo " --product P: Product name used to filter jobs (e.g., microshift, lvm-operator)" >&2 echo " release: OpenShift release version (e.g., 4.22, main)" >&2 exit 1 } @@ - --component) - [[ ${#} -lt 2 ]] && { echo "Error: component requires an argument" >&2; usage; } - component="${2}"; shift 2 ;; + --product) + [[ ${#} -lt 2 ]] && { echo "Error: product requires an argument" >&2; usage; } + product="${2}"; shift 2 ;; @@ - [[ -z "${component}" ]] && { echo "Error: --component is required" >&2; usage; } + [[ -z "${product}" ]] && { echo "Error: --product is required" >&2; usage; } @@ - status) fetch_latest_per_job "${release}" "${component}" ;; - failed) fetch_latest_per_job "${release}" "${component}" | jq '[.[] | select(.status == "failure")]' ;; + status) fetch_latest_per_job "${release}" "${product}" ;; + failed) fetch_latest_per_job "${release}" "${product}" | jq '[.[] | select(.status == "failure")]' ;;🤖 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/prow-jobs-for-release.sh` around lines 30 - 59, The script uses the flag/variable --component and component in main()/usage() which is inconsistent with the shared CLI surface that standardizes on --product; rename all occurrences of --component and the local variable component to --product and product, update the usage text, error messages ("--component is required" → "--product is required") and the command-line parsing case for --component to accept --product instead, and make the corresponding rename in the forwarder script under plugins/microshift-ci/scripts/ so both sides use the same --product/product names.plugins/shared/scripts/create-report.py (2)
638-639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace blind exception catch with explicit handled failures.
Per CONTRIBUTING.md: Python additions must pass
ruff; this block currently violates BLE001 (except Exception). Catch only expected read/encode errors and keep warning+continue behavior.Proposed minimal fix
- except Exception as e: - print(f"WARNING: skipping {png}: {e}", file=sys.stderr) + except OSError as exc: + print(f"WARNING: skipping {png}: {exc}", file=sys.stderr)As per coding guidelines, “Python must follow PEP 8 and pass
ruff.”🤖 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 638 - 639, Replace the broad "except Exception as e" that wraps the PNG read/encode logic with explicit exception types to satisfy ruff BLE001: catch only the expected failures (e.g., FileNotFoundError, PermissionError, OSError, ValueError, and any image-decoding error your image library raises) around the code handling "png" and keep the same warning+continue behavior by printing the message with the caught exception and continuing; ensure you reference and catch the specific image-library exception (e.g., UnidentifiedImageError) if used.
1153-1154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoded
/tmpfor default workdir construction.Per CONTRIBUTING.md: Python additions must pass
ruff; this default path pattern triggers S108. Usetempfile/TMPDIR-aware construction while preserving naming convention.Proposed minimal fix
+import tempfile @@ - if workdir is None: - workdir = f"/tmp/{component}-ci-claude-workdir.{datetime.now().strftime('%y%m%d')}" + if workdir is None: + base_tmp = os.environ.get("TMPDIR", tempfile.gettempdir()) + workdir = os.path.join( + base_tmp, + f"{component}-ci-claude-workdir.{datetime.now().strftime('%y%m%d')}", + )As per coding guidelines, “Python must follow PEP 8 and pass
ruff.”🤖 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 1153 - 1154, The default workdir construction currently hardcodes "/tmp", triggering ruff S108; change the assignment for workdir (when workdir is None) to use tempfile.gettempdir() (TMPDIR-aware) and os.path.join, e.g. workdir = os.path.join(tempfile.gettempdir(), f"{component}-ci-claude-workdir.{datetime.now().strftime('%y%m%d')}"), and ensure tempfile and os are imported at top of create-report.py so the new expression replaces the hardcoded f"/tmp/{component}-ci-claude-workdir.{...}".
🧹 Nitpick comments (5)
plugins/shared/scripts/classify.py (1)
20-20: ⚡ Quick winConsider adding type hints for the shared function.
Since this is shared infrastructure consumed by multiple scripts, adding type hints would improve clarity and enable better IDE support.
📝 Suggested type hints
-def classify_breakdown(stack_layer, step_name="", error_signature=""): +def classify_breakdown(stack_layer: str, step_name: str = "", error_signature: str = "") -> str: """Classify a CI failure as build, test, or infrastructure.🤖 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/classify.py` at line 20, Add explicit type hints to the shared function classify_breakdown: import typing helpers (e.g. Any, Optional) and annotate the signature so stack_layer is typed (e.g. Any), step_name and error_signature are Optional[str] with default "", and the function has an explicit return type (e.g. Any or a more specific type if known); update the def classify_breakdown(...) signature and any related internal variables/returns to match the chosen types to improve IDE support and clarity.plugins/shared/scripts/aggregate.py (2)
45-48: ⚡ Quick winOpen file handles without explicit encoding.
open(...)here relies on the platform's locale encoding (locale.getpreferredencoding(False)), which on some CI images is not UTF-8 and can mangle non-ASCII characters that legitimately show up in Prow logs (e.g., en-dashes, smart quotes, log lines from non-English locales). Since the structured-summary parser is feeding error strings straight into JSON output and HTML reports downstream, a silentUnicodeDecodeError(or worse, mojibake on write) is the kind of regression that only surfaces on a specific runner.Pass
encoding="utf-8"explicitly on allopen()calls in this file.♻️ Proposed change
- with open(filepath, "r") as f: + with open(filepath, "r", encoding="utf-8") as f: @@ - with open(filepath, "r") as f: + with open(filepath, "r", encoding="utf-8") as f: @@ - with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f: @@ - with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f:Also applies to: 83-86, 405-406, 436-437
🤖 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/aggregate.py` around lines 45 - 48, The file opens text files without an explicit encoding (e.g., in parse_structured_summary), which can cause mojibake or UnicodeDecodeError on non-UTF-8 CI images; update every open(...) call in this module to pass encoding="utf-8" (specifically fix the open in parse_structured_summary and the other open calls around the reported areas) so all file reads/writes use UTF-8 consistently and avoid locale-dependent decoding issues.
327-360: 💤 Low valueMutual-exclusion check is dead-code-adjacent and reachable only by accident.
The parse loop already overwrites
modeon each--release/--prs, so by the time control reaches line 358,modereflects only the last one seen — meaning a user who passes--release 4.22 --prssilently runs PR mode without the conflict being noticed in the loop. The post-loop check at line 358 then re-scansargsto detect this, which works but is easy to miss when extending the parser, and themode is not Noneguard is always true at this point (you exit onNonebelow).Consider folding the conflict detection into the parse loop itself, where the state is already being mutated, so the validation lives next to the cause:
♻️ Proposed simplification
elif args[i] == "--release": if i + 1 >= len(args): print("Error: --release requires a version", file=sys.stderr) sys.exit(1) + if mode is not None: + print("Error: --release and --prs are mutually exclusive", file=sys.stderr) + sys.exit(1) mode = "release" release = args[i + 1] i += 2 elif args[i] == "--prs": + if mode is not None: + print("Error: --release and --prs are mutually exclusive", file=sys.stderr) + sys.exit(1) mode = "prs" i += 1 @@ - if mode is not None and args.count("--release") + args.count("--prs") > 1: - print("Error: --release and --prs are mutually exclusive", file=sys.stderr) - sys.exit(1) - if mode is None:Alternatively, replacing the hand-rolled parser with
argparse(mutually-exclusive group +required=Truefor--workdir) would obsolete this whole block. Given the scope of this PR, a follow-up is fine.🤖 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/aggregate.py` around lines 327 - 360, The parser in main() mutates mode while looping through args so the post-loop mutual-exclusion check using args.count("--release") + args.count("--prs") is fragile and can be missed; move the conflict detection into the parse loop: when seeing "--release" or "--prs" inside the loop, check if mode is already set to a different value and if so print the mutual-exclusion error and exit immediately, otherwise set mode/release as currently done; reference the main() function and variables mode, release, args and the existing "--release"/"--prs" branches so the validation lives next to the code that mutates mode (or replace the whole parser with argparse mutually exclusive group in a follow-up).plugins/microshift-ci/skills/doctor/SKILL.md (1)
45-45: ⚡ Quick winPrefer
$PLUGIN_DIRover hardcoded script paths in skill commands.These updated command examples still hardcode
plugins/microshift-ci/..., which reduces portability across environments. Use$PLUGIN_DIR/scripts/doctor.shin all three invocations.As per coding guidelines, “If a skill references
$ARGUMENTS(or tool commands), ensure guidance uses runtime variables for portability (prefer$PLUGIN_DIRover hardcoded paths).”Also applies to: 78-78, 158-158
🤖 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/microshift-ci/skills/doctor/SKILL.md` at line 45, Update the SKILL.md command examples to use the runtime variable instead of hardcoded plugin paths: replace occurrences of "plugins/microshift-ci/scripts/doctor.sh" with "$PLUGIN_DIR/scripts/doctor.sh" in the three invocations (the example at the current diff, plus the ones referenced around lines 78 and 158) so the commands read like "bash $PLUGIN_DIR/scripts/doctor.sh prepare --component microshift --workdir <WORKDIR> <ARGUMENTS> --rebase"; ensure all examples and any guidance referencing the script use $PLUGIN_DIR consistently.plugins/microshift-ci/skills/doctor-refresh/SKILL.md (1)
83-83: ⚡ Quick winUse
$PLUGIN_DIRin the refresh command example for portability.Update this invocation to
$PLUGIN_DIR/scripts/doctor.sh ...instead of a hardcoded repository path.As per coding guidelines, “If a skill references
$ARGUMENTS(or tool commands), ensure guidance uses runtime variables for portability (prefer$PLUGIN_DIRover hardcoded paths).”🤖 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/microshift-ci/skills/doctor-refresh/SKILL.md` at line 83, The example invocation uses a hardcoded repo-relative path; update the command example in SKILL.md so it references the plugin runtime variable by replacing the hardcoded path with $PLUGIN_DIR, e.g. use $PLUGIN_DIR/scripts/doctor.sh refresh --component microshift --workdir $WORKDIR $ARGUMENTS, ensuring $WORKDIR and $ARGUMENTS remain as shown and only the script path (plugins/microshift-ci/scripts/doctor.sh) is replaced with $PLUGIN_DIR/scripts/doctor.sh for portability.
🤖 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/shared/scripts/classify.py`:
- Line 36: The call to stack_layer.lower() can raise AttributeError for None or
non-string values; before calling lower() in the classify logic, validate that
stack_layer is a non-empty string (e.g., check stack_layer is not None and
isinstance(stack_layer, str)), and if the check fails raise a clear TypeError or
ValueError with a descriptive message; after validation use lower() as before.
Ensure you update the code around the stack_layer.lower() usage (the stack_layer
variable and its lower() call) so callers get a deterministic error instead of
an AttributeError.
In `@plugins/shared/scripts/doctor.sh`:
- Line 364: The usage line incorrectly shows --component as optional while
cmd_graphs requires it; update the usage string emitted (the echo that prints
the "graphs" usage) to show --component C as required (remove the surrounding
brackets) or alternatively change cmd_graphs to accept a missing component;
specifically, align the usage echo for "graphs" with the validation in the
cmd_graphs function so the usage reads something like: graphs --component C
[--workdir DIR] [--timezone TZ] to reflect the requirement.
- Line 1: The shebang in doctor.sh must be changed to the required path and the
script hardened: replace "#!/bin/bash" with "#!/usr/bin/bash" at the top of
doctor.sh, add "set -euo pipefail" immediately after the shebang, ensure all
variable expansions in the script are properly quoted, and run/fix any issues
reported by shellcheck so the script passes linting.
In `@plugins/shared/scripts/prow-jobs-for-release.sh`:
- Line 1: Update the script plugins/shared/scripts/prow-jobs-for-release.sh to
conform to CONTRIBUTING.md by replacing the shebang "#!/bin/bash" with
"#!/usr/bin/bash", add "set -euo pipefail" immediately after the shebang, ensure
all variable usages in the script are quoted (e.g., "$var"), and run/fix any
issues reported by shellcheck so the script passes linting.
---
Duplicate comments:
In `@plugins/shared/scripts/create-report.py`:
- Around line 638-639: Replace the broad "except Exception as e" that wraps the
PNG read/encode logic with explicit exception types to satisfy ruff BLE001:
catch only the expected failures (e.g., FileNotFoundError, PermissionError,
OSError, ValueError, and any image-decoding error your image library raises)
around the code handling "png" and keep the same warning+continue behavior by
printing the message with the caught exception and continuing; ensure you
reference and catch the specific image-library exception (e.g.,
UnidentifiedImageError) if used.
- Around line 1153-1154: The default workdir construction currently hardcodes
"/tmp", triggering ruff S108; change the assignment for workdir (when workdir is
None) to use tempfile.gettempdir() (TMPDIR-aware) and os.path.join, e.g. workdir
= os.path.join(tempfile.gettempdir(),
f"{component}-ci-claude-workdir.{datetime.now().strftime('%y%m%d')}"), and
ensure tempfile and os are imported at top of create-report.py so the new
expression replaces the hardcoded f"/tmp/{component}-ci-claude-workdir.{...}".
In `@plugins/shared/scripts/prow-jobs-for-release.sh`:
- Around line 30-59: The script uses the flag/variable --component and component
in main()/usage() which is inconsistent with the shared CLI surface that
standardizes on --product; rename all occurrences of --component and the local
variable component to --product and product, update the usage text, error
messages ("--component is required" → "--product is required") and the
command-line parsing case for --component to accept --product instead, and make
the corresponding rename in the forwarder script under
plugins/microshift-ci/scripts/ so both sides use the same --product/product
names.
---
Nitpick comments:
In `@plugins/microshift-ci/skills/doctor-refresh/SKILL.md`:
- Line 83: The example invocation uses a hardcoded repo-relative path; update
the command example in SKILL.md so it references the plugin runtime variable by
replacing the hardcoded path with $PLUGIN_DIR, e.g. use
$PLUGIN_DIR/scripts/doctor.sh refresh --component microshift --workdir $WORKDIR
$ARGUMENTS, ensuring $WORKDIR and $ARGUMENTS remain as shown and only the script
path (plugins/microshift-ci/scripts/doctor.sh) is replaced with
$PLUGIN_DIR/scripts/doctor.sh for portability.
In `@plugins/microshift-ci/skills/doctor/SKILL.md`:
- Line 45: Update the SKILL.md command examples to use the runtime variable
instead of hardcoded plugin paths: replace occurrences of
"plugins/microshift-ci/scripts/doctor.sh" with "$PLUGIN_DIR/scripts/doctor.sh"
in the three invocations (the example at the current diff, plus the ones
referenced around lines 78 and 158) so the commands read like "bash
$PLUGIN_DIR/scripts/doctor.sh prepare --component microshift --workdir <WORKDIR>
<ARGUMENTS> --rebase"; ensure all examples and any guidance referencing the
script use $PLUGIN_DIR consistently.
In `@plugins/shared/scripts/aggregate.py`:
- Around line 45-48: The file opens text files without an explicit encoding
(e.g., in parse_structured_summary), which can cause mojibake or
UnicodeDecodeError on non-UTF-8 CI images; update every open(...) call in this
module to pass encoding="utf-8" (specifically fix the open in
parse_structured_summary and the other open calls around the reported areas) so
all file reads/writes use UTF-8 consistently and avoid locale-dependent decoding
issues.
- Around line 327-360: The parser in main() mutates mode while looping through
args so the post-loop mutual-exclusion check using args.count("--release") +
args.count("--prs") is fragile and can be missed; move the conflict detection
into the parse loop: when seeing "--release" or "--prs" inside the loop, check
if mode is already set to a different value and if so print the mutual-exclusion
error and exit immediately, otherwise set mode/release as currently done;
reference the main() function and variables mode, release, args and the existing
"--release"/"--prs" branches so the validation lives next to the code that
mutates mode (or replace the whole parser with argparse mutually exclusive group
in a follow-up).
In `@plugins/shared/scripts/classify.py`:
- Line 20: Add explicit type hints to the shared function classify_breakdown:
import typing helpers (e.g. Any, Optional) and annotate the signature so
stack_layer is typed (e.g. Any), step_name and error_signature are Optional[str]
with default "", and the function has an explicit return type (e.g. Any or a
more specific type if known); update the def classify_breakdown(...) signature
and any related internal variables/returns to match the chosen types to improve
IDE support and clarity.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3cfc84ca-c6d8-440b-9fa4-2016dd9c1f05
📒 Files selected for processing (21)
plugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/classify.pyplugins/microshift-ci/scripts/classify.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/doctor-refresh/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/classify.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/download-jobs.shplugins/shared/scripts/prow-jobs-for-release.sh
✅ Files skipped from review due to trivial changes (7)
- plugins/microshift-ci/scripts/prow-jobs-for-release.sh
- plugins/microshift-ci/skills/continue-session/SKILL.md
- plugins/microshift-ci/scripts/doctor.sh
- plugins/microshift-ci/scripts/doctor.sh
- plugins/microshift-ci/scripts/aggregate.py
- plugins/microshift-ci/scripts/classify.py
- plugins/microshift-ci/scripts/create-report.py
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/microshift-ci/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/prow-jobs-for-release.sh
- plugins/microshift-ci/scripts/create-report.py
- plugins/shared/scripts/download-jobs.sh
kasturinarra
force-pushed
the
lvms_ci_doctor_p1
branch
from
6cb8b7e to
aefc9b9
Compare
Copy doctor.sh, aggregate.py, classify.py, create-report.py, download-jobs.sh, and prow-jobs-for-release.sh from plugins/microshift-ci/scripts/ to plugins/shared/scripts/. Fix shebangs to use #!/usr/bin/bash per CONTRIBUTING.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the original scripts in plugins/microshift-ci/scripts/ with symlinks pointing to plugins/shared/scripts/. MicroShift behavior remains identical. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add --component parameter to shared scripts so they can be reused by other components (LVMS). Update all SKILL.md files to pass --component microshift explicitly. Changes: - doctor.sh: add --component to prepare/finalize/graphs/refresh - prow-jobs-for-release.sh: add --component for job name filtering - create-report.py: add --component with COMPONENT_TITLES validation - aggregate.py: use plain classify import, make --workdir mandatory - download-jobs.sh: minor docstring update - SKILL.md files: pass --component microshift Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kasturinarra
force-pushed
the
lvms_ci_doctor_p1
branch
from
aefc9b9 to
d2eb39f
Compare
- Rename product_title to component_title in create-report.py - Replace 'products' with 'components' in aggregate.py comments - Add --component to graphs usage in doctor.sh header comment - Make component a positional arg in prow-jobs-for-release.sh - Update doctor.sh caller to match new positional interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/shared/scripts/create-report.py (2)
1107-1144: ⚡ Quick winConsider using
argparsefor CLI parsing.The hand-rolled loop works, but
argparsewould replace ~30 lines of manual parsing with a declarative spec, provide--helpautomatically, enforce thechoices=COMPONENT_TITLES.keys()constraint (subsuming the explicit check at 1142–1144), and align with Python best practices noted in CONTRIBUTING.md.♻️ Sketch
- workdir = None - releases_arg = None - component = None - - args = sys.argv[1:] - i = 0 - while i < len(args): - if args[i] == "--workdir": - if i + 1 >= len(args): - print("Error: --workdir requires an argument", file=sys.stderr) - sys.exit(1) - workdir = args[i + 1] - i += 2 - elif args[i] == "--component": - if i + 1 >= len(args): - print("Error: --component requires an argument", file=sys.stderr) - sys.exit(1) - component = args[i + 1] - i += 2 - elif args[i].startswith("-"): - print(f"Unknown option: {args[i]}", file=sys.stderr) - sys.exit(1) - else: - releases_arg = args[i] - i += 1 - - if not releases_arg: - print("Usage: create-report.py --component <component> [--workdir DIR] <release1,release2,...>", file=sys.stderr) - sys.exit(1) - - if not component: - print("Error: --component is required", file=sys.stderr) - sys.exit(1) - - if component not in COMPONENT_TITLES: - print(f"Error: unsupported component '{component}'. Supported: {', '.join(COMPONENT_TITLES)}", file=sys.stderr) - sys.exit(1) + import argparse + parser = argparse.ArgumentParser(description="Generate an HTML report from analyze-ci JSON files.") + parser.add_argument("--component", required=True, choices=sorted(COMPONENT_TITLES), + help="Component name (e.g. microshift, lvm-operator).") + parser.add_argument("--workdir", default=None, help="Directory containing analyze-ci JSON files.") + parser.add_argument("releases", help="Comma-separated list of release versions (e.g. 4.21,4.22).") + ns = parser.parse_args() + component, workdir, releases_arg = ns.component, ns.workdir, ns.releases🤖 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 1107 - 1144, The CLI parsing in main() is manually implemented; replace it with argparse to simplify and harden parsing: create an ArgumentParser, add --workdir (optional), add --component as a required argument with choices=COMPONENT_TITLES.keys(), and add a single positional argument for the comma-separated releases; then use the parsed args (args.workdir, args.component, args.releases) and remove the hand‑rolled loop and the explicit component membership/error checks and usage prints so argparse will provide --help/validation automatically.
1251-1266: ⚡ Quick winSpecify
encoding="utf-8"when writing report artifacts.The HTML and JSON outputs embed user-derived strings (issue titles, JIRA summaries, error messages) that can contain non-ASCII characters. Relying on the platform default encoding (
locale.getpreferredencoding(False)) is fragile across hosts/containers and is something ruff/PEP 8 idioms recommend making explicit. Same applies to the file reads at lines 252, 269, 287, and 635 for symmetry.♻️ Proposed change
- with open(bugs_summary_path, "w") as f: + with open(bugs_summary_path, "w", encoding="utf-8") as f: json.dump(bugs_tab_data, f, indent=2) @@ - with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f: f.write(html_content)🤖 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 1251 - 1266, The file writes for report artifacts currently use open(...) without an explicit encoding which can corrupt non-ASCII user-provided strings; update the write calls to use encoding="utf-8" (e.g., the open call that writes bugs_summary_path and the open call that writes output_path/html) and likewise update any earlier file reads that open inputs to use encoding="utf-8" (the reader calls that load data used by generate_html, bugs_tab_data, pr_data, pr_error, etc.) so all file opens consistently specify encoding="utf-8".
🤖 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/microshift-ci/skills/doctor-refresh/SKILL.md`:
- Line 83: The SKILL example invocation uses the old flag --component; update
the example command (the "bash ... doctor.sh refresh" invocation shown) to use
--product microshift instead of --component microshift so the documented flag
matches the refactor contract and actual CLI behavior (ensure the string "bash
plugins/microshift-ci/scripts/doctor.sh refresh --component microshift --workdir
<WORKDIR> <ARGUMENTS>" is changed to use --product).
---
Nitpick comments:
In `@plugins/shared/scripts/create-report.py`:
- Around line 1107-1144: The CLI parsing in main() is manually implemented;
replace it with argparse to simplify and harden parsing: create an
ArgumentParser, add --workdir (optional), add --component as a required argument
with choices=COMPONENT_TITLES.keys(), and add a single positional argument for
the comma-separated releases; then use the parsed args (args.workdir,
args.component, args.releases) and remove the hand‑rolled loop and the explicit
component membership/error checks and usage prints so argparse will provide
--help/validation automatically.
- Around line 1251-1266: The file writes for report artifacts currently use
open(...) without an explicit encoding which can corrupt non-ASCII user-provided
strings; update the write calls to use encoding="utf-8" (e.g., the open call
that writes bugs_summary_path and the open call that writes output_path/html)
and likewise update any earlier file reads that open inputs to use
encoding="utf-8" (the reader calls that load data used by generate_html,
bugs_tab_data, pr_data, pr_error, etc.) so all file opens consistently specify
encoding="utf-8".
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a423c902-dec3-4379-814c-3b1612ac55b6
📒 Files selected for processing (21)
plugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/aggregate.pyplugins/microshift-ci/scripts/classify.pyplugins/microshift-ci/scripts/classify.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/create-report.pyplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/download-jobs.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/scripts/prow-jobs-for-release.shplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/doctor-refresh/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/classify.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/download-jobs.shplugins/shared/scripts/prow-jobs-for-release.sh
✅ Files skipped from review due to trivial changes (9)
- plugins/microshift-ci/scripts/doctor.sh
- plugins/microshift-ci/scripts/create-report.py
- plugins/microshift-ci/skills/continue-session/SKILL.md
- plugins/microshift-ci/skills/doctor/SKILL.md
- plugins/microshift-ci/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/classify.py
- plugins/microshift-ci/scripts/prow-jobs-for-release.sh
- plugins/microshift-ci/scripts/aggregate.py
- plugins/microshift-ci/scripts/classify.py
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/microshift-ci/scripts/doctor.sh
- plugins/shared/scripts/prow-jobs-for-release.sh
- plugins/shared/scripts/classify.py
- plugins/shared/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/download-jobs.sh
- plugins/microshift-ci/scripts/prow-jobs-for-release.sh
- plugins/shared/scripts/doctor.sh
- plugins/shared/scripts/aggregate.py
kasturinarra
changed the title
|
/lgtm |
1 similar comment
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, kasturinarra 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 |
2 participants
Summary
doctor.sh,prow-jobs-for-release.sh,create-report.py,aggregate.py,classify.py,download-jobs.sh) fromplugins/microshift-ci/scripts/toplugins/shared/scripts/--componentflag so they can be reused by other components (LVMS in PR 2)#!/usr/bin/bashper CONTRIBUTING.md--component microshiftContext
This is PR 1 of 2 for the LVMS CI Doctor feature. PR 2 (depends on this) will add the
plugins/lvms-ci/plugin that symlinks to these same shared scripts.Changes
plugins/shared/scripts/doctor.sh--componentparamplugins/shared/scripts/prow-jobs-for-release.shplugins/shared/scripts/create-report.py--componentparam withCOMPONENT_TITLESvalidationplugins/shared/scripts/aggregate.py--workdirmandatoryplugins/shared/scripts/classify.pyplugins/shared/scripts/download-jobs.shplugins/microshift-ci/scripts/(6 files)../../shared/scripts/plugins/microshift-ci/skills/doctor/SKILL.md--component microshiftplugins/microshift-ci/skills/continue-session/SKILL.md--component microshiftplugins/microshift-ci/skills/doctor-refresh/SKILL.md--component microshiftTest plan
readlink -f plugins/microshift-ci/scripts/doctor.sh)/microshift-ci:doctor 4.22and confirm identical output to pre-refactor behaviorbash plugins/microshift-ci/scripts/doctor.shshows updated usage with--componentflagpython3 plugins/microshift-ci/scripts/aggregate.pyfindsclassify.pyvia symlinkcreate-report.py --component invalidshows supported components error🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation