net, udn: Change passt binding plugin to core binding#4033
Open
frenzyfriday wants to merge 1 commit intoRedHatQE:mainfrom
Open
net, udn: Change passt binding plugin to core binding#4033frenzyfriday wants to merge 1 commit intoRedHatQE:mainfrom
frenzyfriday wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRenames the UDN passt binding constant to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
frenzyfriday
force-pushed
the
passt-core-binding
branch
from
b682abf to
1d25fc4
Compare
|
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. |
openshift-virtualization-qe-bot-4
requested review from
Anatw,
EdDev,
RoniKishner,
dshchedr and
vsibirsk
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
frenzyfriday
force-pushed
the
passt-core-binding
branch
from
1d25fc4 to
7490f9e
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
Author
|
/wip |
frenzyfriday
force-pushed
the
passt-core-binding
branch
2 times, most recently
from
16386a8 to
580c2ff
Compare
|
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. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/vm/spec.py (1)
77-77:⚠️ Potential issue | 🟠 MajorHIGH: Remove the newly introduced linter suppression on Line 77
Line 77 adds a new
# noqa: N815. New suppressions are disallowed because they hide naming violations and accumulate lint debt. Keep internal field naming compliant (e.g.,passt_binding) and map to external API keypasstBindingat the serialization boundary.Suggested direction
`@dataclass` class Interface: name: str masquerade: dict[Any, Any] | None = None bridge: dict[Any, Any] | None = None sriov: dict[Any, Any] | None = None - passtBinding: dict[Any, Any] | None = None # noqa: N815 + passt_binding: dict[Any, Any] | None = None binding: NetBinding | None = None state: str | None = None# then map passt_binding -> "passtBinding" when building the manifest dict # (where VMSpec/Interface is serialized).As per coding guidelines: "
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore, or# pylint: disable— if linter complains, fix the code instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/vm/spec.py` at line 77, The new field name uses a disallowed noqa; rename the field from passtBinding to a PEP8-compliant passt_binding (update its declaration, any uses, and constructor/assignments) and remove the "# noqa: N815" suppression, then update the serialization boundary (where VMSpec/Interface is converted to the manifest/dict) to emit the external key "passtBinding" from the internal passt_binding value so external API stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/vm/spec.py`:
- Line 77: The new field name uses a disallowed noqa; rename the field from
passtBinding to a PEP8-compliant passt_binding (update its declaration, any
uses, and constructor/assignments) and remove the "# noqa: N815" suppression,
then update the serialization boundary (where VMSpec/Interface is converted to
the manifest/dict) to emit the external key "passtBinding" from the internal
passt_binding value so external API stays unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df8132ea-f16e-4ae2-8d92-e346daa6fe49
📒 Files selected for processing (3)
libs/net/udn.pylibs/vm/spec.pytests/network/user_defined_network/test_user_defined_network_passt.py
openshift-virtualization-qe-bot
added
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
and removed
tox:verify-tc-requirement-polarion:running
Contributor
Author
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
As a part of the passt-beta VEP [1] passt is now a core binding instead of a binding plugin. This commit updates the udn passt tests so that they use VMs with interface that has passtBinding instead of binding:passt [1] https://github.com/kubevirt/enhancements/blob/main/veps/sig-network/passt/passt-beta.md
frenzyfriday
force-pushed
the
passt-core-binding
branch
from
580c2ff to
30d76b4
Compare
Contributor
Author
|
Last change is a rebase |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/vm/spec.py (1)
77-77:⚠️ Potential issue | 🟠 MajorHIGH: Remove the newly added
# noqa: N815onInterface.passtBinding.Suppressions here create lint debt and bypass the repo rule; use a compliant internal name and map it to the external manifest key during serialization.
Proposed direction
- passtBinding: dict[Any, Any] | None = None # noqa: N815 + passt_binding: dict[Any, Any] | None = NoneThen map
passt_binding→passtBindingin the VM manifest serialization path.As per coding guidelines:
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore, or# pylint: disable— if linter complains, fix the code instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/vm/spec.py` at line 77, Remove the `# noqa: N815` suppression and rename the Interface attribute from `passtBinding` to a linter-compliant internal name `passt_binding` (update the attribute definition and all internal references), then update the VM manifest serialization path (the code that emits the VM manifest / serialize function for Interface) to map the internal `passt_binding` back to the external manifest key `passtBinding` so the external schema remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/vm/spec.py`:
- Line 77: Remove the `# noqa: N815` suppression and rename the Interface
attribute from `passtBinding` to a linter-compliant internal name
`passt_binding` (update the attribute definition and all internal references),
then update the VM manifest serialization path (the code that emits the VM
manifest / serialize function for Interface) to map the internal `passt_binding`
back to the external manifest key `passtBinding` so the external schema remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 205d3867-3415-43d3-a32e-a977023ff5a5
📒 Files selected for processing (3)
libs/net/udn.pylibs/vm/spec.pytests/network/user_defined_network/test_user_defined_network_passt.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As a part of the passt-beta VEP [1] passt is now a core binding instead of a binding plugin.
This commit updates the udn passt tests so that they use VMs with interface that has passtBinding instead of binding:passt
[1] https://github.com/kubevirt/enhancements/blob/main/veps/sig-network/passt/passt-beta.md
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
passtBindingis the name of the interface binding according to KubeVirt schemaSince this is a camel case and python requires it to be a snake case I have added noqa at
passtBinding: dict[Any, Any] | None = None # noqa: N815following this exampleTested locally on a 4.22 cluster
jira-ticket:
CNV-76560
Summary by CodeRabbit