Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Skip setting vlan when parameter is not present #296

Merged
merged 2 commits into from
May 9, 2024

Conversation

mlguerrero12
Copy link
Contributor

Fixes #291

@coveralls
Copy link

coveralls commented Apr 5, 2024

Pull Request Test Coverage Report for Build 8924595799

Details

  • 37 of 41 (90.24%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.1%) to 49.569%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/sriov/sriov.go 4 8 50.0%
Totals Coverage Status
Change from base Build 8835333415: 2.1%
Covered Lines: 632
Relevant Lines: 1275

💛 - Coveralls

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!

please just extend a bit more our unit test to cover all the cases related to vlans

*conf.Vlan,
*conf.VlanQoS,
sriovtypes.VlanProtoInt[*conf.VlanProto]

to be sure we call the function when needed (vlan is not empty)

@mlguerrero12
Copy link
Contributor Author

mlguerrero12 commented Apr 16, 2024

great work!

please just extend a bit more our unit test to cover all the cases related to vlans

*conf.Vlan,
*conf.VlanQoS,
sriovtypes.VlanProtoInt[*conf.VlanProto]

to be sure we call the function when needed (vlan is not empty)

Thanks @SchSeba .

There are many cases related to vlan in config_test.go. Regarding the calling of the function, yes, there is one missing case, so I added unit tests for ApplyVFConfig which covers all optional parameters.

@mlguerrero12
Copy link
Contributor Author

@Eoghan1232, @e0ne, please have a look

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nice work!

Fixes k8snetworkplumbingwg#291

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
@mlguerrero12
Copy link
Contributor Author

Hi @adrianchiris, could you please have a look at this? Thanks!

@Eoghan1232
Copy link
Collaborator

working my way through my emails now, so getting around to the PR's

@SchSeba SchSeba merged commit 20c67e0 into k8snetworkplumbingwg:master May 9, 2024
10 of 11 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip setting VLAN ID when VLAN is 0 - breaks on VF trunk policy
4 participants