Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Delete ConfIFNames parameter #269

Merged

Conversation

mlguerrero12
Copy link
Contributor

@mlguerrero12 mlguerrero12 commented Jun 5, 2023

This parameter is only used in a check that is not valid because only a single interface name is used by the plugin. Additionally, the func GetVFLinkNames was renamed to GetVFLinkName to reflect that a single name is returned.

@mlguerrero12 mlguerrero12 changed the title Fix number of VF interface names Delete VF interface names length check Jun 5, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5177878515

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 38.113%

Totals Coverage Status
Change from base Build 5122498764: 0.01%
Covered Lines: 420
Relevant Lines: 1102

💛 - Coveralls

Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

Changes look good to me.
I'm not sure if this can break some backward compatibility: NetConf is serialized during cmdAdd and deserialized during cmdDel.

@SchSeba , @adrianchiris, @Eoghan1232 Do you have any comment about this?

pkg/sriov/sriov.go Show resolved Hide resolved
@mlguerrero12
Copy link
Contributor Author

thanks @zeeke for your comment. You made me realize that we don't need this parameter at all. The interface name is taken from the var args. I will delete it.

And no, it won't break backwards compatibility for the above reason.

@mlguerrero12 mlguerrero12 changed the title Delete VF interface names length check Delete ConfIFNames parameter Jun 15, 2023
Copy link
Collaborator

@Eoghan1232 Eoghan1232 left a comment

Choose a reason for hiding this comment

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

this change lgtm, makes sense, thanks!

Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM

This parameter is only used in a check that is not valid
because only a single interface name is used by the plugin.
Additionally, the func GetVFLinkNames was renamed to
GetVFLinkName to reflect that a single name is returned.

Signed-off-by: Marcelo Guerrero <[email protected]>
@adrianchiris adrianchiris merged commit 868c852 into k8snetworkplumbingwg:master Jul 19, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants