Rename x86_64 to amd64 for architecture consistency#3789
Rename x86_64 to amd64 for architecture consistency#3789vsibirsk merged 2 commits intoRedHatQE:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request systematically renames the x86_64 architecture constant to amd64/AMD_64 across configuration files, core utilities, and tests. The changes propagate from constant definitions through configuration markers, environment variables, and unit test expectations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can generate a title for your PR based on the changes.Add ✨ 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 |
1 similar comment
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can generate a title for your PR based on the changes.Add ✨ 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can generate a title for your PR based on the changes.Add ✨ 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
📝 WalkthroughWalkthroughThis pull request systematically replaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
3 similar comments
📝 WalkthroughWalkthroughThis pull request systematically replaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
📝 WalkthroughWalkthroughThis pull request systematically replaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
📝 WalkthroughWalkthroughThis pull request systematically replaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tox.ini (1)
8-16: 🧹 Nitpick | 🔵 TrivialConsider renaming the testenv to match the new terminology.
The environment is still named
pytest-check-x86(line 8) while the arch value is nowamd64(line 16). This mismatch could confuse contributors. Consider renaming topytest-check-amd64for consistency, and updating theenvliston line 2 accordingly.utilities/architecture.py (1)
20-33:⚠️ Potential issue | 🟡 MinorBreaking change:
x86_64is no longer accepted as a valid architecture — fix the error message grammar.The core behavioral change is confirmed:
x86_64has been removed from the allowed set; onlyamd64,arm64, ands390xare now accepted. Sinceget_cluster_architecture()reads from both theOPENSHIFT_VIRTUALIZATION_TEST_IMAGES_ARCHenv var andkubernetes.io/archnode labels:
- Node labels: Kubernetes already reports
amd64for x86-64 nodes, so no issue there.- Env var: Any CI pipeline, developer setup, or external caller still passing
x86_64will now get aValueError.However, fix the grammar error in the error message on line 33: change
"architecture in not supported"to"architecture is not supported". The test on line 94 oftest_architecture.pyexpects the current incorrect grammar, so update that test assertion as well.Note: The
X86_64constant is still defined inutilities/constants.pyand used inutilities/infra.pyfor convertingplatform.machine()results to Kubernetes arch constants, so this constant need not be removed.utilities/unittests/test_architecture.py (1)
88-97:⚠️ Potential issue | 🟠 MajorHIGH: Missing test for
x86_64as now-unsupported architecture.The existing unsupported test only checks a generic
"unsupported"string. Since this PR explicitly removesx86_64from the valid architectures inget_cluster_architecture(), you should add a test confirming that"x86_64"is now rejected with aValueError. This is a meaningful behavioral change — anyone who previously hadOPENSHIFT_VIRTUALIZATION_TEST_IMAGES_ARCH=x86_64in their environment will hit this error, and the test suite should document and verify that.Suggested addition after line 97
def test_get_cluster_architecture_x86_64_unsupported(self): """Test that x86_64 is no longer a supported architecture value""" with ( patch.dict(os.environ, {"OPENSHIFT_VIRTUALIZATION_TEST_IMAGES_ARCH": "x86_64"}), pytest.raises( ValueError, match="x86_64 architecture in not supported", ), ): get_cluster_architecture()
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 593-594: Update the trailing comment that currently reads
"verified on x86_64 platforms" to match the marker being applied by
item.add_marker(marker=AMD_64); change it to "verified on amd64 platforms" (or
"verified on x86_64/amd64 platforms") so the comment and the AMD_64 marker stay
consistent and avoid confusion.
In `@pytest.ini`:
- Line 45: Update the pytest marker description that currently reads "amd64:
Tests that can run on x86_64-based cluster" to use the standardized term; change
the description to something like "amd64: Tests that can run on amd64-based
cluster" so the marker name and description consistently reference amd64 (locate
the marker entry in pytest.ini where the "amd64:" description is defined).
|
/approve |
|
/approve |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3592. Overlapping filesutilities/constants.py |
|
/lgtm |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
9 participants
Short description:
Align architecture naming with Kubernetes/KubeVirt conventions
Assisted by: Cursor
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Tests
Chores