-
Notifications
You must be signed in to change notification settings - Fork 7
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
🌱 Fix tests. #47
🌱 Fix tests. #47
Conversation
These e2e tests seem to be hard-coded to a specific environment. I tried to get them running with a kind cluster, but failed.
hcloud/util.go
Outdated
@@ -101,6 +104,9 @@ func getRobotServerByID(c robotclient.Client, id int, node *corev1.Node) (*model | |||
} | |||
|
|||
// check whether name matches - otherwise this server does not belong to the respective node anymore | |||
if server == 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.
this shouldn't happen right? If there is an error code for "server not found"?
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.
If the error code was "server not found", then a nil pointer exception happens, because server was nil.
I made the old code easier to read. It now looks like this:
server, err := c.ServerGet(id)
if models.IsError(err, models.ErrorCodeServerNotFound) {
return nil, nil
}
if err != nil {
hcops.HandleRateLimitExceededError(err, node)
return nil, fmt.Errorf("%s: %w", op, err)
}
// check whether name matches - otherwise this server does not belong to the respective node anymore
if server.Name != node.Name {
return nil, nil
}
The old check looked like this:
if err != nil && !models.IsError(err, models.ErrorCodeServerNotFound) {
Is that ok for you?
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 looks good
Fix tests.
Up to now, they failed like this: