USHIFT-7041: MicroShift / LVMS CI Doctor - make log file names and copy procedure more generic#79580
Conversation
|
@ggiguash: This pull request references USHIFT-7041 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughTwo edge-tooling doctor scripts switch to per-phase Claude logs and rsync-based artifact collection; exit-time validation now checks the latest ChangesCI Doctor Script Logging Infrastructure Refactor
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 full review |
✅ Actions performedFull review triggered. |
|
/pj-rehearse periodic-ci-openshift-eng-edge-tooling-main-microshift-ci-doctor periodic-ci-openshift-eng-edge-tooling-main-lvms-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.sh`:
- Around line 18-23: The rsync filter order is wrong: the specific directory
excludes (--exclude='lvm-operator/' and --exclude='artifacts/') are placed after
the catch-all --exclude='*' so they never take effect; update the rsync
invocation (the rsync -a ... command in
openshift-edge-tooling-lvms-ci-doctor-commands.sh) so that the directory
excludes appear before the general --exclude='*' (or convert to explicit
--filter rules) while preserving the existing --include='*/' and file includes,
ensuring lvm-operator/ and artifacts/ are excluded during traversal.
In
`@ci-operator/step-registry/openshift/edge-tooling/microshift-ci/doctor/openshift-edge-tooling-microshift-ci-doctor-commands.sh`:
- Around line 21-26: The rsync filter order is wrong so directory excludes
(--exclude='microshift/' and --exclude='artifacts/') are ineffective; update the
rsync invocation that copies from "${WORKDIR}/" to "${ARTIFACT_DIR}/" so the
directory-specific excludes appear before the general --exclude='*' (e.g., keep
--include='*/' first, then move --exclude='microshift/' and
--exclude='artifacts/' before the --include='*.html'/*.json'/*.txt'/*.log' and
the final --exclude='*') to ensure files inside those directories are not
copied.
🪄 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: 07b9a5fb-af51-4f17-a98a-f5945c1b839e
📒 Files selected for processing (2)
ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.shci-operator/step-registry/openshift/edge-tooling/microshift-ci/doctor/openshift-edge-tooling-microshift-ci-doctor-commands.sh
|
/pj-rehearse periodic-ci-openshift-eng-edge-tooling-main-microshift-ci-doctor periodic-ci-openshift-eng-edge-tooling-main-lvms-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse ack |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-edge-tooling-main-microshift-ci-doctor |
|
@kasturinarra: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
Looks like there is a problem with the change. |
|
@ggiguash, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/unhold |
|
/pj-rehearse periodic-ci-openshift-eng-edge-tooling-main-microshift-ci-doctor periodic-ci-openshift-eng-edge-tooling-main-lvms-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@ggiguash, |
|
@ggiguash, |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.sh (2)
46-48: 💤 Low valueChange "WARNING" to "ERROR" for consistency.
The message says "WARNING" but the code returns 1 (error exit code). The message should say "ERROR" to accurately reflect that this is a hard failure.
📝 Proposed fix
if [ ! -f "${CLAUDE_DOCTOR_LOG}" ]; then - echo "WARNING: Log file '${CLAUDE_DOCTOR_LOG}' not found" + echo "ERROR: Log file '${CLAUDE_DOCTOR_LOG}' not found" return 1 fi🤖 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 `@ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.sh` around lines 46 - 48, The echo message for the missing log file uses "WARNING" but the code returns a non-zero error, so update the message to "ERROR" for consistency: locate the conditional that checks [ ! -f "${CLAUDE_DOCTOR_LOG}" ] and change the echo string from "WARNING: Log file '${CLAUDE_DOCTOR_LOG}' not found" to "ERROR: Log file '${CLAUDE_DOCTOR_LOG}' not found" so the printed severity matches the exit status.
3-3: ⚡ Quick winRemove
set -xto comply with step registry guidelines.As per coding guidelines, step registry script files must use
set -euo pipefailwithout-xas the default, enabling-xonly when actively debugging. While this script doesn't handle sensitive data, the guideline establishes a consistent standard for all step registry scripts.📋 Proposed fix
-set -x🤖 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 `@ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.sh` at line 3, Replace the top-level "set -x" in openshift-edge-tooling-lvms-ci-doctor-commands.sh with the standard step-registry safety flags by removing the "set -x" token and adding "set -euo pipefail" at the script start; if you need verbose debugging, enable "-x" only conditionally (e.g., when a DEBUG env var is set) so the explicit "set -x" is no longer present.
🤖 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
`@ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.sh`:
- Around line 46-48: The echo message for the missing log file uses "WARNING"
but the code returns a non-zero error, so update the message to "ERROR" for
consistency: locate the conditional that checks [ ! -f "${CLAUDE_DOCTOR_LOG}" ]
and change the echo string from "WARNING: Log file '${CLAUDE_DOCTOR_LOG}' not
found" to "ERROR: Log file '${CLAUDE_DOCTOR_LOG}' not found" so the printed
severity matches the exit status.
- Line 3: Replace the top-level "set -x" in
openshift-edge-tooling-lvms-ci-doctor-commands.sh with the standard
step-registry safety flags by removing the "set -x" token and adding "set -euo
pipefail" at the script start; if you need verbose debugging, enable "-x" only
conditionally (e.g., when a DEBUG env var is set) so the explicit "set -x" is no
longer present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ac31eba8-b5da-4948-87db-661cd6121026
📒 Files selected for processing (2)
ci-operator/step-registry/openshift/edge-tooling/lvms-ci/doctor/openshift-edge-tooling-lvms-ci-doctor-commands.shci-operator/step-registry/openshift/edge-tooling/microshift-ci/doctor/openshift-edge-tooling-microshift-ci-doctor-commands.sh
|
/pj-rehearse ? |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@ggiguash, |
|
@ggiguash, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse ? |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@ggiguash, |
|
@ggiguash, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
cb5a005 to
b611dfc
Compare
b611dfc to
e3fc5b9
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse periodic-ci-openshift-eng-edge-tooling-main-microshift-ci-doctor periodic-ci-openshift-eng-edge-tooling-main-lvms-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, 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 |
|
/pj-rehearse ack |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@ggiguash: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary by CodeRabbit
This PR updates CI Doctor step scripts in the OpenShift CI release repo for two edge-tooling jobs (MicroShift CI Doctor and LVMS CI Doctor) to standardize log file names, make artifact copying more robust, and tighten session-success validation.
What changed in practical terms
Impact
Files changed (high level)
Review notes