USHIFT-6793: MicroShift CI Doctor - PCP tool prerequisites and GitHub token fix#77966
Conversation
|
@ggiguash: This pull request references USHIFT-6793 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI_Script as CI Script
participant GH_Ext as "gh-token extension"
participant GH_API as "GitHub REST API"
participant Env as "Process Env"
CI_Script->>GH_Ext: download & verify gh-token extension
CI_Script->>GH_Ext: generate GitHub App JWT
GH_Ext-->>CI_Script: JWT
CI_Script->>GH_API: GET /app/installations (with JWT) -> installation_id
GH_API-->>CI_Script: installation_id
CI_Script->>GH_API: POST /app/installations/{id}/access_tokens (with JWT)
GH_API-->>CI_Script: installation access token
CI_Script->>Env: export GITHUB_TOKEN=<access token>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 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 |
|
@ggiguash: This pull request references USHIFT-6793 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh (1)
106-106: Consider usingpip install --userfor consistency.Line 176 in this same script uses
pip install uv --user --upgrade. Using an unqualifiedpip install matplotlibhere installs system-wide (or fails under PEP 668 "externally-managed-environment" restrictions on newer distros). For consistency and robustness, consider:♻️ Proposed change
- pip install matplotlib + pip install matplotlib --user🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh` at line 106, Replace the unqualified "pip install matplotlib" with a user-local install to match the other installs and avoid system-wide/PEP 668 issues: change the command to use the --user flag (e.g., "pip install --user matplotlib"), and optionally add --upgrade if you want the same upgrade behavior as "pip install uv --user --upgrade".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh`:
- Line 107: Replace the plain sudo invocation so it fails fast if a password
prompt would be required: update the command `sudo dnf install -y
pcp-export-pcp2json` to use non-interactive sudo (e.g. `sudo -n dnf install -y
pcp-export-pcp2json`) or prepend a quick check like `sudo -n true || { echo
"passworded sudo not allowed"; exit 1; }` before the install; ensure the change
targets the line containing `sudo dnf install -y pcp-export-pcp2json`.
---
Nitpick comments:
In
`@ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh`:
- Line 106: Replace the unqualified "pip install matplotlib" with a user-local
install to match the other installs and avoid system-wide/PEP 668 issues: change
the command to use the --user flag (e.g., "pip install --user matplotlib"), and
optionally add --upgrade if you want the same upgrade behavior as "pip install
uv --user --upgrade".
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f62f7a6e-1108-4b15-abeb-5fbd76821ae4
📒 Files selected for processing (1)
ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
e776cd7 to
7bb7611
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@ggiguash: This pull request references USHIFT-6793 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh (1)
106-107: Consider pinning versions for reproducible CI.
pip install matplotlibanddnf install -y pcp-export-pcp2jsonboth pull whatever is latest at job time, which can introduce silent behavior changes in CI Doctor runs. For consistency with the pinnedgh-token(line 55) andmcp-atlassian@0.21.0(line 185) patterns already used in this file, consider pinningmatplotlibto a known-good version (e.g.pip install 'matplotlib==X.Y.Z'). Also note thepip installhere runs without--user, unlikepip install uv --user --upgradeinconfigure_claude(line 176) — harmless if running as root in the container, but worth a consistency pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh` around lines 106 - 107, Pin the package versions and make pip usage consistent: replace the unpinned "pip install matplotlib" with a specific version (e.g. "pip install 'matplotlib==X.Y.Z'") and pin the DNF package by installing a known-good release (ensure the exact pcp-export-pcp2json package version or repository tag), and align pip invocation style with the existing "pip install uv --user --upgrade" used in configure_claude by either adding "--user" or documenting why system-wide install is required; update the "pip install matplotlib" and "dnf install -y pcp-export-pcp2json" lines accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh`:
- Around line 106-107: Pin the package versions and make pip usage consistent:
replace the unpinned "pip install matplotlib" with a specific version (e.g. "pip
install 'matplotlib==X.Y.Z'") and pin the DNF package by installing a known-good
release (ensure the exact pcp-export-pcp2json package version or repository
tag), and align pip invocation style with the existing "pip install uv --user
--upgrade" used in configure_claude by either adding "--user" or documenting why
system-wide install is required; update the "pip install matplotlib" and "dnf
install -y pcp-export-pcp2json" lines accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e1065a0-22d5-4cfe-8fd9-6ff790b14c18
📒 Files selected for processing (1)
ci-operator/step-registry/openshift/microshift/claude/ci-doctor/openshift-microshift-claude-ci-doctor-commands.sh
7bb7611 to
879b5b1
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
61450d8 to
b429c9f
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
🧩 Analysis chain🏁 Script executed: Length of output: 5473
Full Code ReviewSummary of Changes (Both Commits)Commit 1 – "Install PCP tool prerequisites"
Commit 2 – "Fix token generation to use installation ID"
Issues Found🔴 Critical –
|
4a58f5b to
bae5728
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
bae5728 to
7206065
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
7206065 to
6dc4161
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-ci-doctor |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
6dc4161 to
b8e17da
Compare
|
/pj-rehearse periodic-ci-openshift-microshift-main-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: |
|
@ggiguash: The following test failed, say
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. |
|
/pj-rehearse ack |
|
@ggiguash: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, pmtk 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 |
f5d1b0d
into
openshift:main
… token fix (openshift#77966) * Install PCP tool prerequisites * Fix GitHub App token usage
… token fix (openshift#77966) * Install PCP tool prerequisites * Fix GitHub App token usage
Also addresses USHIFT-6863
Summary by CodeRabbit