[Virt] Add suffix for s390x template names for Common templates tests #921
Conversation
## Walkthrough
Architecture-awareness is added to template list generation functions by introducing a required `cluster_arch` parameter. Template lists and names are adapted based on architecture, particularly for "s390x". The pytest fixture dynamically determines the node CPU architecture for accurate template expectations. A new constant `S390X` is added to CPU architecture constants.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| tests/virt/cluster/common_templates/general/test_base_template.py | Updated template list functions to accept `cluster_arch`, adapted template naming and filtering for "s390x" architecture, and modified pytest fixture to detect node CPU architecture dynamically and pass it to these functions. |
| utilities/constants.py | Added new CPU architecture constant `S390X = "s390x"` and `S390X_TEMPLATE_SUFFIX = "-s390x"` alongside existing constants. |
## Suggested labels
`verified`, `size/XS`, `can-be-merged`
## Suggested reviewers
- vsibirsk
- dshchedr
- kbidarkar
- akri3i
- rnetser📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)tests/virt/cluster/common_templates/general/test_base_template.py (1)
🪛 Pylint (3.3.7)tests/virt/cluster/common_templates/general/test_base_template.py[error] 145-145: No value for argument 'nodes' in function call (E1120) ⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
polarion-jenkins
added
tox:verify-tc-requirement-polarion:running
|
/build-and-push-container --build-arg OPENSHIFT_PYTHON_WRAPPER_COMMIT=06faf24b83a33dec0cd228b9b690a1d440f240f2 |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-921 published |
ghost
added
the
|
/retest build-container |
|
jdao-rh is not allowed to run retest commands.
|
|
/retest build-container |
|
@rnetser is there a way to run the verified check? It has been waiting to run for a few days now |
@jdao-rh the verified check is executed when the PR is marked as @RoniKishner @geetikakay please review this PR |
| rhel_major_releases_list = ["7", "8", "9"] | ||
| template_suffix = "" | ||
| if cluster_arch == S390X: | ||
| template_suffix = f"-{S390X}" |
There was a problem hiding this comment.
This string appears also in line 109 and 137, why not doing it as const?
| def get_fedora_templates_list(cluster_arch): | ||
| template_suffix = f"-{S390X}" if cluster_arch == S390X else "" | ||
| return [ | ||
| f"fedora-{workload}-{flavor}{template_suffix}" |
There was a problem hiding this comment.
Make template suffix as const and with f"-{S390X}" as value and then remove the template_suffix var and use it directly:
| f"fedora-{workload}-{flavor}{template_suffix}" | |
| f"fedora-{workload}-{flavor}{S390_TEMPLATE_SUFFIX if cluster_arch == S390X else ''}" |
There was a problem hiding this comment.
Thanks for the suggestion, will update the PR with your suggested changes 👍
|
|
||
|
|
||
| def get_windows_templates_list(): | ||
| def get_windows_templates_list(cluster_arch): | ||
| if cluster_arch == "s390x": |
| centos_releases_list = ["-stream9"] | ||
| return [ | ||
| f"centos{release}-{workload}-{flavor}" | ||
| f"centos{release}-{workload}-{flavor}{template_suffix}" |
There was a problem hiding this comment.
ditto, use directly:
| f"centos{release}-{workload}-{flavor}{template_suffix}" | |
| f"centos{release}-{workload}-{flavor}{-{S390_TEMPLATE_SUFFIX if cluster_arch == S390X else ''}" |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Short description:
Adds support of test on s390x by adding correct template names for
virt/cluster/commontemplate/generaltestsMore details:
What this PR does / why we need it:
Allows virt/cluster/commontemplate/general to be run against s390x arch
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit