-
Notifications
You must be signed in to change notification settings - Fork 47
specs-go: fix minumum version check for IntelRdt. #294
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
specs-go: fix minumum version check for IntelRdt. #294
Conversation
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
82eedf4 to
40d236b
Compare
cmd/cdi/cmd/validate.go
Outdated
| } | ||
|
|
||
| for _, v := range cache.ListVendors() { | ||
| for _, s := range cache.GetVendorSpecs(v) { |
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.
I can see this nested loop in other places as well. Would it make sense to wrap it in function?
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.
You mean something like
// ForeachSpec iterates through all Specs by vendor, calling the given
// callback function for every Spec. It stops iterating early if the function
// returns true.
func ForeachSpec(fn func(spec *cdi.Spec) bool) {
for _, v := range cache.ListVendors() {
for _, s := range cache.GetVendorSpecs(v) {
done := fn(s)
if done {
return
}
}
}
}I'm fine doing that, but if we decide so then let's do it in a separate 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.
Yes, this is what I was thinking about. I'm ok with doing it in a separate PR if you think it makes sense to do.
bart0sh
left a comment
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
marquiz
left a comment
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 @klihub
How's the netdevice stuff related to this? Shouldn't those be a separate PR (or then adjust the PR title and summary accordingly)?
nit: one commit message title s/minumum/minimum/
| } | ||
| } | ||
|
|
||
| for _, dev := range spec.Devices { |
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.
Good catch @klihub. Thanks.
40d236b to
8efc0ff
Compare
@marquiz Argh, it is not related otherwise than me having those stacked on the same branch because they need to touch the same function in
Thanks for spotting it. Fixed that one, too. |
Require 1.1.0 if at least one of the global or device specific container edits use IntelRdt. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
8efc0ff to
09c7768
Compare
This slipped through our review...
Add minimum version check unit test for IntelRdt. Fix check to require
1.1.0if at least one of the global or device specific container edits use IntelRdt.