IPU: update the name of upgraded image #4402
Conversation
WalkthroughA conditional guard was added in the in-place upgrade test to normalize version strings before constructing the upgraded image name: if Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (1)
qemu/tests/in_place_upgrade.py (1)
124-125: Replace generic exception handler with specific error handling.The bare
except Exceptionis too broad and will catch unexpected errors (AttributeError, KeyError, etc.), making debugging difficult. Catch only the exceptions you expect and re-raise or handle others appropriately.try: image_name = params.objects("images")[0] image_params = params.object_params(image_name) image_path = params.get("images_base_dir", data_dir.get_data_dir()) old_name = storage.get_image_filename(image_params, image_path) if len(post_rhel_ver) == 3 and len(pre_rhel_ver) == 2: pre_rhel_ver += "0" upgraded_name = old_name.replace(pre_rhel_ver, post_rhel_ver) process.run(params.get("image_clone_command") % (old_name, upgraded_name)) - except Exception as error: + except (OSError, subprocess.CalledProcessError) as error: test.log.warning("Failed to rename upgraded image:%s", str(error))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qemu/tests/in_place_upgrade.py(1 hunks)
🔇 Additional comments (1)
qemu/tests/in_place_upgrade.py (1)
120-122: The review comment raises substantive concerns that are partially confirmed by code analysis. The version string normalization logic does rely on a very specific condition (len==3 and len==2) that is undocumented, and thestr.replace()approach does pose a risk of unintended substring matches in image paths.However, I cannot definitively verify whether this condition covers all supported upgrade scenarios (e.g., whether RHEL 7→8, 8→9, or other combinations produce different version string lengths). The test configuration files that would contain actual version examples were not found in the repository search.
Based on the available evidence:
- Confirmed: The condition is specific to one length mismatch scenario and lacks documentation
- Confirmed: Using
str.replace()for version substitution could match unintended substrings- Unable to confirm: Whether other version length combinations exist that require handling
- Unable to confirm: Whether the "0" padding is the only correct normalization needed
MiriamDeng
changed the title
| if len(post_rhel_ver) == 3 and len(pre_rhel_ver) == 2: | ||
| pre_rhel_ver += "0" |
There was a problem hiding this comment.
Can you provide commit message for this change?
And I cannot see any mapping between image name and guest variants.
There was a problem hiding this comment.
Updated, thanks ! please help to review it again. @PaulYuuu
|
Test results from rhel810-rhel980 |
|
Test it from 9 to 10 |
Make sure the name of image includes 3 numbers Signed-off-by: Miriam Deng <[email protected]>
MiriamDeng
force-pushed
the
fix_upgraded_image_name
branch
from
529f635 to
2ade518
Compare
3 participants
Signed-off-by: Miriam Deng [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.