NO-ISSUE: images/tools/Dockerfile: Install iotop, if that fails fall back to iotop-c#2216
NO-ISSUE: images/tools/Dockerfile: Install iotop, if that fails fall back to iotop-c#2216sdodson merged 1 commit intoopenshift:mainfrom
Conversation
WalkthroughReplaces conditional iotop detection and variable assignment with a static inclusion of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The way we've dealt with problems like this in the past where binaries shift from one package to another is to just name the path to the binary in the installspec. i.e. something like please test this suggestion, it's just a suggestion. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
images/tools/Dockerfile (1)
10-51:⚠️ Potential issue | 🟠 MajorAvoid reusing install specs as
rpm -Vverify targets.
/usr/sbin/iotopis a valid installspec foryum, butrpm -Vtreats arguments as package names by default, not file paths. Passing/usr/sbin/iotoptorpm -V ... $INSTALL_PKGSwill fail verification because/usr/sbin/iotopis not a valid package name. Split install specs from verify targets, and resolve the iotop owner package beforerpm -V.Proposed fix
-RUN INSTALL_PKGS="\ +RUN INSTALL_SPECS="\ bash-completion \ bc \ bind-utils \ blktrace \ crash \ e2fsprogs \ ethtool \ file \ fio \ git \ glibc-utils \ gzip \ hwloc \ /usr/sbin/iotop \ iproute \ iputils \ jq \ less \ ltrace \ net-tools \ nmap-ncat \ parted \ pciutils \ procps-ng \ psmisc \ perf \ python3 \ sos \ s-nail \ strace \ stress-ng \ subscription-manager \ sysstat \ tcpdump \ tmux \ util-linux \ vim-enhanced \ wget \ xfsprogs \ " && \ - yum -y install $INSTALL_PKGS && rpm -V --nogroup --nosize --nofiledigest --nomtime --nomode $INSTALL_PKGS && yum clean all && rm -rf /var/cache/* + yum -y install $INSTALL_SPECS && \ + IOTOP_PKG="$(rpm -qf /usr/sbin/iotop 2>/dev/null || rpm -qf /usr/bin/iotop)" && \ + VERIFY_PKGS="$(echo "$INSTALL_SPECS" | sed 's#/usr/sbin/iotop##g') $IOTOP_PKG" && \ + rpm -V --nogroup --nosize --nofiledigest --nomtime --nomode $VERIFY_PKGS && \ + yum clean all && rm -rf /var/cache/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/tools/Dockerfile` around lines 10 - 51, The current Dockerfile builds INSTALL_PKGS (used by yum) but then passes that same list to rpm -V, which expects package names not file paths like /usr/sbin/iotop; fix by creating a separate VERIFY_PKGS list containing only package names (e.g., replace file paths with their owning package such as iotop) or programmatically resolve owners for entries like /usr/sbin/iotop with rpm -qf before verification, then call rpm -V on VERIFY_PKGS (leave yum -y install using INSTALL_PKGS unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@images/tools/Dockerfile`:
- Around line 10-51: The current Dockerfile builds INSTALL_PKGS (used by yum)
but then passes that same list to rpm -V, which expects package names not file
paths like /usr/sbin/iotop; fix by creating a separate VERIFY_PKGS list
containing only package names (e.g., replace file paths with their owning
package such as iotop) or programmatically resolve owners for entries like
/usr/sbin/iotop with rpm -qf before verification, then call rpm -V on
VERIFY_PKGS (leave yum -y install using INSTALL_PKGS unchanged).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
images/tools/Dockerfile
|
/override ci/prow/e2e-agnostic-ovn-cmd ci/prow/e2e-aws-oc-ote ci/prow/e2e-aws-oc-ote-serial ci/prow/e2e-aws-ovn ci/prow/e2e-aws-ovn-serial-1of2 ci/prow/e2e-aws-ovn-serial-2of2 ci/prow/e2e-aws-ovn-upgrade Build is already broken, as long as the images test passes this is an improvement. |
|
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-agnostic-ovn-cmd, ci/prow/e2e-aws-oc-ote, ci/prow/e2e-aws-oc-ote-serial, ci/prow/e2e-aws-ovn, ci/prow/e2e-aws-ovn-serial-1of2, ci/prow/e2e-aws-ovn-serial-2of2, ci/prow/e2e-aws-ovn-upgrade 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn-serial-2of2 |
|
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aws-ovn-serial-2of2 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/images |
|
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/images 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: sdodson, wking 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 |
|
@sdodson: This pull request explicitly references no jira issue. 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. |
|
/verified by CI |
|
@sdodson: This PR has been marked as verified by 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. |
|
/override ci/prow/e2e-agnostic-ovn-cmd ci/prow/e2e-aws-ovn ci/prow/e2e-aws-ovn-upgrade |
|
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-agnostic-ovn-cmd, ci/prow/e2e-aws-ovn, ci/prow/e2e-aws-ovn-upgrade 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 kubernetes-sigs/prow repository. |
|
@sdodson: 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. |
… iotop-c
Summary by CodeRabbit