feat: update generate_template_labels method #2604
Conversation
|
Warning Rate limit exceeded@vsibirsk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded an Architecture namespace and ARCHITECTURE label constant; modified Template.generate_template_labels(os, workload, flavor, architecture=None) to append Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/template.py (1)
68-68: Remove unnecessary parentheses.The parentheses around the f-string are not needed.
-(f"{Template.Labels.WORKLOAD}/{getattr(Template.Workload, workload.upper())}=true"), +f"{Template.Labels.WORKLOAD}/{getattr(Template.Workload, workload.upper())}=true",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/template.py(2 hunks)
🔇 Additional comments (2)
ocp_resources/template.py (2)
15-15: LGTM! Clean addition of architecture label constant.The new ARCHITECTURE constant follows the established pattern and is properly namespaced.
65-73: The label format is correct and follows KubeVirt conventions.The ARCHITECTURE label uses a different format intentionally:
template.kubevirt.io/architecture={value}is the official KubeVirt convention for architecture labels, while OS, WORKLOAD, and FLAVOR follow the pattern{domain.kubevirt.io}/{value}=truefor discrete template attribute labels.This is not an inconsistency—KubeVirt's template label conventions deliberately distinguish between:
- Attribute labels with values encoded in keys (OS, WORKLOAD, FLAVOR): multiple possible label keys, one per valid option
- Flexible value labels (ARCHITECTURE): single key with variable architecture values
No changes needed.
vsibirsk
force-pushed
the
vk-multiarch-template
branch
from
0b2058e to
053c3e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/template.py (1)
71-72: Consider adding validation for the architecture parameter.The
workloadandflavorparameters are validated usinggetattrwith corresponding class constants (Template.Workload,Template.Flavor), but thearchitectureparameter is used directly without validation. For consistency and to catch invalid values early, consider whether architecture should also have a set of valid constants or validation.If there's a finite set of supported architectures, you could add a class like:
class Architecture: AMD64 = "amd64" ARM64 = "arm64" PPC64LE = "ppc64le" S390X = "s390x"Then validate the architecture parameter:
if architecture: - labels.append(f"{Template.Labels.ARCHITECTURE}={architecture}") + labels.append(f"{Template.Labels.ARCHITECTURE}={getattr(Template.Architecture, architecture.upper())}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/template.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ocp_resources/template.py (1)
ocp_resources/resource.py (3)
NamespacedResource(1574-1684)ApiGroup(468-581)labels(1211-1218)
🔇 Additional comments (1)
ocp_resources/template.py (1)
15-15: LGTM!The
ARCHITECTUREconstant follows the same pattern as other label constants in the class and correctly uses theTEMPLATE_KUBEVIRT_IOAPI group.
|
a/approve |
|
/approve |
redhat-qe-bot
added
needs-rebase
and removed
approved-rnetser
commented-coderabbitai[bot]
lgtm-rnetser
labels
on multi-arch clusters multiple templates of same OS (with diff arch) exists added optional architecture param
vsibirsk
force-pushed
the
vk-multiarch-template
branch
from
b99804f to
1186737
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ocp_resources/template.py (1)
15-20: LGTM! Architecture constants follow Kubernetes conventions.The new
ARCHITECTURElabel constant andArchitecturenamespace with lowercase values (amd64, arm64, s390x) are well-defined and match standard Kubernetes architecture labels. Thearchitectureparameter is properly validated viagetattr()in thegenerate_template_labelsmethod.Note: The architecture label format (
template.kubevirt.io/architecture=<value>) differs from the OS/WORKLOAD/FLAVOR pattern (subdomain.template.kubevirt.io/<value>=true). Consider whether alignment with the existing label format convention is needed for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/template.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T16:42:29.245Z
Learnt from: servolkov
Repo: RedHatQE/openshift-python-wrapper PR: 2490
File: ocp_resources/route_advertisements.py:53-65
Timestamp: 2025-08-11T16:42:29.245Z
Learning: In the openshift-python-wrapper project, when raising MissingRequiredArgumentError, the convention is to include the "self." prefix in the argument name (e.g., `MissingRequiredArgumentError(argument="self.advertisements")`). This pattern is established in the code generator template at `class_generator/manifests/class_generator_template.j2` and followed consistently across most resource classes.
Applied to files:
ocp_resources/template.py
🧬 Code graph analysis (1)
ocp_resources/template.py (1)
ocp_resources/resource.py (3)
NamespacedResource(1574-1684)ApiGroup(468-581)labels(1211-1218)
🔇 Additional comments (2)
ocp_resources/template.py (2)
70-70: No verification needed—method has no callers in the codebase.The
architectureparameter addition is backward compatible (optional, at end of parameter list, conditionally used), but no callers exist to verify. No action required.
71-78: Verify label format compatibility before release.The label format has changed:
- OS/WORKLOAD/FLAVOR labels now include
=truesuffix:os.template.kubevirt.io/rhel9=true- Architecture label uses
key=valueformat:template.kubevirt.io/architecture=amd64If these labels are consumed by external tools or selectors for filtering or matching, verify that they can handle the new format. The inconsistent formats between OS/WORKLOAD/FLAVOR (using
/value=true) and ARCHITECTURE (using=value) may also warrant standardization.Positive note: Architecture now includes validation via
getattr()(line 77).Minor inconsistency: OS parameter (line 72) lacks validation, unlike workload, flavor, and architecture parameters.
|
/verified |
|
/approve |
5 participants
Short description:
on multi-arch clusters multiple templates of same OS (with diff arch) exists
added optional architecture param
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.