-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add validation for xnames in BootparametersPost and enhance tests #52
Add validation for xnames in BootparametersPost and enhance tests #52
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.
Looks good. According to the OpenAPI spec, PUT only updates and does not create new so I think implementing the check just for POST is fine for now.
Hosts []string `json:"hosts,omitempty"` | ||
Hosts []string `json:"hosts,omitempty"` // This list of hosts must be xnames |
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.
(No change requested here, just commenting for posterity)
Unfortunately, it looks like there may have been premature optimization here as this field (according to the comment above) is supposed to support xnames and "special names" (i.e. groups). However, the current BSS does not support boot groups with Postgres (yet). This may change with a solution to #50. However for now, this is fine as-is.
@@ -450,6 +450,16 @@ func BootparametersPost(w http.ResponseWriter, r *http.Request) { | |||
fmt.Sprintf("Bad Request: %s", err)) | |||
return | |||
} | |||
// Check that the xnames are valid | |||
err = args.CheckXnames() |
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.
(No change requested here, just commenting for posterity)
See comment in pkg/bssTypes/types.go on line 61. When a solution comes for the mentioned issue, this may change to be conditional in the future.
Tested this on our cluster:
|
This adds validation of xnames for bss using the Cray-HPE xname library and adds tests to validate the behavior I believe is requested in #51