-
Notifications
You must be signed in to change notification settings - Fork 545
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
test: skip image polling test if running in a kind cluster #2425
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -918,3 +918,21 @@ func HaveMessage(goal string) gtypes.GomegaMatcher { | |
return plan.Status.Message | ||
}, ContainSubstring(goal)) | ||
} | ||
|
||
func inKind(client operatorclient.ClientInterface) (bool, error) { | ||
nodes, err := client.KubernetesInterface().CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) | ||
if err != nil { | ||
// error finding nodes | ||
return false, err | ||
} | ||
Comment on lines
+924
to
+927
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be easier to just default to false, and avoid returning an error entirely, if we run into an error while listing out the nodes. This would allow call sites to avoid needing to check whether there's an error (and improve readability decreasing the number of conditional chains). Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I had it that way originally, but a failure listing the nodes is an error. I think it should be treated as one, even if it makes the function more complex |
||
for _, node := range nodes.Items { | ||
if !strings.HasPrefix(node.GetName(), "kind-") { | ||
continue | ||
} | ||
if !strings.HasSuffix(node.GetName(), "-control-plane") { | ||
continue | ||
} | ||
return true, nil | ||
} | ||
return false, nil | ||
Comment on lines
+928
to
+937
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Testing this logic out locally and it seemed to catch the default kind node name and any other variations, like `kind-123432-control-plane, and correctly filtered out cluster environment we don't care about in the context of this 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.
We may want to also use context.Background() instead of context.TODO() here.
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, I wasn't sure about whether to thread a context through -- might be ok leaving as is for now