-
Notifications
You must be signed in to change notification settings - Fork 582
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
Unit tests and update to quickstart guide #354
Conversation
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.
Thank you for this PR! It must increase the code coverage, awesome!
I add some minor comment, so could you please take a look into it?
Once changed, let's merge it.
k8sclient/k8sclient_test.go
Outdated
Expect(err).NotTo(HaveOccurred()) | ||
}) | ||
|
||
/* Still figuring this one out. need to make "setPodNetworkAnnotation throw an error */ |
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.
Golang prefers to use '//' than '/* */' for comments. Could you please change it?
k8sclient/k8sclient_test.go
Outdated
} | ||
}` | ||
|
||
delegate, err := types.LoadDelegateNetConf([]byte(conf), nil, "0000:00:00.0") |
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.
In case of non-sriov case, 3rd argument of LoadDelegateNetConf() could be "" (empty) instead of "0000:00:00.0".
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 changed this in my most recent commit, but I'm not sure what "non-sriov case" means. Can you explain so I will know where/why such changes should be made for future? Thank you
kubeletclient/kubeletclient_test.go
Outdated
@@ -115,6 +115,15 @@ var _ = Describe("Kubelet resource endpoint data read operations", func() { | |||
_, err := GetResourceClient() | |||
Expect(err).NotTo(HaveOccurred()) | |||
}) | |||
|
|||
It("should fail with missing file", func() { | |||
// os.Mkdir("tempdir", 0644) |
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.
Could you remove commented out code (if it is not necessary)?
kubeletclient/kubeletclient_test.go
Outdated
|
||
It("should fail with missing file", func() { | ||
// os.Mkdir("tempdir", 0644) | ||
kubeletSocket = "Andrew Bays" |
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 example to us but not so universal 😆 We may use simple string for that, like "sampleSocketString".
Added additional unit tests and added a stub section for Network Status Annotations in quickstart.md