wip#6908
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test ? |
WalkthroughA ChangesBootupd Disabling in Kickstart
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 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 |
|
/test e2e-aws-tests |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/test e2e-aws-tests |
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 `@test/kickstart-templates/includes/main-prologue.cfg`:
- Around line 13-19: The sed command that modifies the have_bootupd function in
util.py does not verify success before proceeding. Add an error check
immediately after the sed command to ensure the modification was applied
successfully. Since this is critical boot functionality in the %pre phase, the
script should fail if the sed operation fails rather than silently continuing.
Check the exit status of the sed command and exit with an error code if it
fails, or use the sed command with a flag that will cause the script to fail if
the operation does not complete as expected.
- Line 14: Add logging after the UTIL_FILE variable assignment to identify and
display which util.py file was found and will be patched. After the line that
assigns UTIL_FILE from the find command, add an echo statement that outputs the
file path being patched to /dev/console for visibility. This will help with
debugging when test failures occur across different RHEL/Fedora versions and
will make it clear whether the correct util.py was located or if the pattern
matching failed.
🪄 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: 6a1c03d4-e677-445e-9d2b-4d6cd35c6463
📒 Files selected for processing (1)
test/kickstart-templates/includes/main-prologue.cfg
| %pre --log=/dev/console | ||
| UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1) | ||
| if [ -n "${UTIL_FILE}" ] && grep -q 'def have_bootupd' "${UTIL_FILE}"; then | ||
| sed -i '/def have_bootupd/a\ return False' "${UTIL_FILE}" | ||
| find "$(dirname "${UTIL_FILE}")" -name '__pycache__' -type d -exec rm -rf {} + 2>/dev/null || true | ||
| fi | ||
| %end |
There was a problem hiding this comment.
Verify sed success and consider fallback for missing targets.
The script silently proceeds even if sed fails to modify the function. Since this is a %pre phase that affects critical boot paths, undetected modification failures could leave bootupd enabled during installation.
🛡️ Proposed improvement to verify sed success
%pre --log=/dev/console
UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1)
if [ -n "${UTIL_FILE}" ] && grep -q 'def have_bootupd' "${UTIL_FILE}"; then
- sed -i '/def have_bootupd/a\ return False' "${UTIL_FILE}"
+ if ! sed -i '/def have_bootupd/a\ return False' "${UTIL_FILE}"; then
+ echo "ERROR: Failed to patch have_bootupd() in ${UTIL_FILE}" >> /dev/console
+ exit 1
+ fi
find "$(dirname "${UTIL_FILE}")" -name '__pycache__' -type d -exec rm -rf {} + 2>/dev/null || true
fi
%end📝 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.
| %pre --log=/dev/console | |
| UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1) | |
| if [ -n "${UTIL_FILE}" ] && grep -q 'def have_bootupd' "${UTIL_FILE}"; then | |
| sed -i '/def have_bootupd/a\ return False' "${UTIL_FILE}" | |
| find "$(dirname "${UTIL_FILE}")" -name '__pycache__' -type d -exec rm -rf {} + 2>/dev/null || true | |
| fi | |
| %end | |
| %pre --log=/dev/console | |
| UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1) | |
| if [ -n "${UTIL_FILE}" ] && grep -q 'def have_bootupd' "${UTIL_FILE}"; then | |
| if ! sed -i '/def have_bootupd/a\ return False' "${UTIL_FILE}"; then | |
| echo "ERROR: Failed to patch have_bootupd() in ${UTIL_FILE}" >> /dev/console | |
| exit 1 | |
| fi | |
| find "$(dirname "${UTIL_FILE}")" -name '__pycache__' -type d -exec rm -rf {} + 2>/dev/null || true | |
| fi | |
| %end |
🤖 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 `@test/kickstart-templates/includes/main-prologue.cfg` around lines 13 - 19,
The sed command that modifies the have_bootupd function in util.py does not
verify success before proceeding. Add an error check immediately after the sed
command to ensure the modification was applied successfully. Since this is
critical boot functionality in the %pre phase, the script should fail if the sed
operation fails rather than silently continuing. Check the exit status of the
sed command and exit with an error code if it fails, or use the sed command with
a flag that will cause the script to fail if the operation does not complete as
expected.
| # causes both the storage module (installs GRUB to MBR/ESP) and the rpm-ostree | ||
| # module (configures grub.cfg) to use their standard non-bootupd code paths. | ||
| %pre --log=/dev/console | ||
| UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Simulate the find pattern used in the kickstart
# Expected: Exactly one file found and verified
UTIL_FILE=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | head -1)
if [ -z "${UTIL_FILE}" ]; then
echo "ERROR: No util.py found. Check pyanaconda package and version."
exit 1
fi
echo "Found util.py at: ${UTIL_FILE}"
# Count matches to detect silent failures from head -1
MATCH_COUNT=$(find /usr/lib*/python3*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree -name util.py 2>/dev/null | wc -l)
if [ "${MATCH_COUNT}" -gt 1 ]; then
echo "WARNING: Multiple matches found (${MATCH_COUNT}), using first: ${UTIL_FILE}"
fi
# Verify expected content
if ! grep -q 'def have_bootupd' "${UTIL_FILE}"; then
echo "ERROR: have_bootupd() not found in ${UTIL_FILE}"
exit 1
fi
echo "SUCCESS: util.py is valid and contains have_bootupd()"Repository: openshift/microshift
Length of output: 127
🏁 Script executed:
# Read the target file to understand context
cat -n test/kickstart-templates/includes/main-prologue.cfg | head -30Repository: openshift/microshift
Length of output: 1704
🏁 Script executed:
# Check the overall structure of the kickstart template directory
find test/kickstart-templates -type f | head -20Repository: openshift/microshift
Length of output: 1155
Add logging to identify which util.py file was patched for better debugging.
The path pattern is fragile, but your code already validates file existence and content before patching (line 15). However, explicitly logging which file was identified and modified would help with test failures across different RHEL/Fedora versions. For example:
echo "Patching have_bootupd() in ${UTIL_FILE}" > /dev/console
This ensures test failures can reveal whether a different util.py was patched or the pattern failed entirely.
🤖 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 `@test/kickstart-templates/includes/main-prologue.cfg` at line 14, Add logging
after the UTIL_FILE variable assignment to identify and display which util.py
file was found and will be patched. After the line that assigns UTIL_FILE from
the find command, add an echo statement that outputs the file path being patched
to /dev/console for visibility. This will help with debugging when test failures
occur across different RHEL/Fedora versions and will make it clear whether the
correct util.py was located or if the pattern matching failed.
|
@pacevedom: 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. |
1 participant
Summary by CodeRabbit