-
Notifications
You must be signed in to change notification settings - Fork 482
Conversation
Signed-off-by: Zach Arnold <[email protected]>
Signed-off-by: Zach Arnold <[email protected]>
Signed-off-by: Zach Arnold <[email protected]>
Signed-off-by: Zach Arnold <[email protected]>
The test timeout on MacOS is not related to my code I believe. Please let me know what I need to do to get this merged. |
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.
There is an unrelated test flake causing CI failures at the moment.
For testing, I could see two options:
-
Create a pod via
testutil.CreatePod
, adding a fake node name to the pod spec, usingoptions.DashConfig.Objectstore
to list all known pods withstore.Key{Kind: "Pod", APIVersion: "v1"}
, use gomockobjectStore.EXPECT().List...
, then check if the table generated this way filters for the node name correctly. -
Import
testClient "k8s.io/client-go/kubernetes/fake"
and populate that with the pods. Then filter the mock data for node name as it doesn't support field selectors (see Fake client doesn't filter Pods on FieldSelector kubernetes/client-go#326)
Currently inclined to say go with the first option since it can actually test if octant is filtering for the correct pods and could potentially avoid a call to the API server (although trading off performance if there are lots of pods).
@@ -325,8 +330,10 @@ func createNodeConditionsView(node *corev1.Node) (*component.Table, error) { | |||
return table, nil | |||
} | |||
|
|||
const GB = float64(1073741824) |
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 will likely see usage in other places in printer especially around volumes/storage. Perhaps util
is a better place for this.
@zparnold were there any other questions you had about this change besides the testing suggestions? |
HI there, sorry I've really been struggling with the testing angle of this. I may need some hand holding here unfortunately. It certainly works internal to my company (we just use my copy for our clusters) but that's obviously not a test case. I don't suppose I can push the broken code I have now and have you tell me how to go in the right direction? Or even let's meet so I can see what I'm doing wrong? This type of a complex testing scenario I just have very little experience with and all my exploration of the codebase has not revealed which direction I should go. |
@zparnold sure, we'd love to help out, are you able to join our slack channel https://kubernetes.slack.com/archives/CM37M9FCG we can coordinate there, if not, you can use https://calendly.com/wwitzel3/octant-office-hours to setup a block of time for us to sync via Zoom. |
@zparnold Ping again on this as 0.17 is likely to be released next week and I'd to see this included. I'd also be okay with writing tests on this if needed. |
Let's go ahead with writing some tests for this. We'll make sure to keep the contribution history for Zach and add who ever does the tests as a co-author. |
Thank you guys for being willing to do this for me. I’m sorry I kind of dropped the ball here. Feel free to attribute the code to whomever no worries. I hope to improve my Go skillz at some point, but open source contribution is sadly not yet something I can do a ton of at this time. Really appreciate your supportiveness! High five for community! |
Signed-off-by: Zach Arnold [email protected]
What this PR does / why we need it:
Adds a "Pods" table to the Node overview so that people can see which pods are scheduled onto the node
Which issue(s) this PR fixes
Special notes for your reviewer:
I'm not quite sure how to mock the Kubernetes Client to create a test for this, I'd need some help on that.
Release note: