Skip to content
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

Joshd/sc 88868/reject requests for vm clusters with 1 nodes #326

Merged

Conversation

jdewinne
Copy link
Member

Next to doing a validation on number of nodes, it also fixes some other issues.
https://www.loom.com/share/cd576538ffa44be8a538f11c46d8b9df?sid=0839438f-8349-4a9e-8b92-cd92b5932f3a

This is a beta feature, with some known limitations:
- K3s, Kind, kurl, helmvm and Openshift clusters only support a single node.
- EKS clusters may report ready before the nodes are completely online.
- kurl is the only supported distribution that can be upgraded.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line should be a limitation of the cluster upgrade command, not cluster create

Copy link
Member

@divolgin divolgin Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a fair warning here so it's not a surprise later. Maybe wording can be improved.

return nil, ve, nil
// if err is APIError and the status code is 400, then we have a validation error
// and we can return the validation error
if apiErr, ok := err.(platformclient.APIError); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors.Cause(err).(platformclient.APIError)

}
return nil, fmt.Errorf("%s", errors.New(strings.Join(ve.Errors, ",")))
return nil, fmt.Errorf("%s", ve.Message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors.New(ve.Message)

divolgin
divolgin previously approved these changes Sep 15, 2023
@divolgin divolgin merged commit 7fe51d3 into main Sep 15, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants