-
Notifications
You must be signed in to change notification settings - Fork 111
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
Skip vf configuration when not found in VF group #539
Skip vf configuration when not found in VF group #539
Conversation
Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 6904899519
💛 - Coveralls |
// Do not configure VF which is not part of any VF Group. | ||
if !found { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially filtering the list of addresses to process. Now, the body of the loop does much too much imo. Let's take the opportunity to refactor this whole function to have a more proper way to filter the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, please have a look again
Thanks for your PR,
To skip the vendors CIs use one of:
|
db58834
to
16efbac
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
16efbac
to
ce298b2
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
ce298b2
to
7390d13
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
created a test in #541 to show the issue. I added here as well. It should pass now. |
OCP job passed (with new added test). There is a problem setting up the k8s job (not related to this pr). |
Thanks for your PR,
To skip the vendors CIs use one of:
|
if sriovnetworkv1.StringInArray(group.DeviceType, DpdkDrivers) { | ||
dpdkDriver = group.DeviceType | ||
} | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for moving this check !
@@ -1071,6 +1071,83 @@ var _ = Describe("[sriov] operator", func() { | |||
})) | |||
}) | |||
|
|||
It("Should configure the mtu only for vfs which are part of the partition", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving the test into this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @mlguerrero12
No description provided.