Orchestration: Add support for resource_type endpoints #1806
Conversation
|
Build failed.
|
| // ListOpts allows the filtering of collections through the API. | ||
| type ListOpts struct { | ||
| // Filters the resource type list by a regex on the name. | ||
| Pattern string `q:"name"` |
There was a problem hiding this comment.
The Field name should match the JSON tag: Name.
There was a problem hiding this comment.
I used a different name because this API has proved confusing to users: it isn't checking for that resource type by name, but filtering the list by a regex. I'd change the API if I could, but although that horse has bolted it would be nice to avoid perpetuating the confusion here. Would something like NameRegEx be acceptable?
|
Build failed.
|
There was a problem hiding this comment.
@zaneb Thank you for the updates. To be clear, I agree entirely with your opinion about implementing this in a modern/sane way that makes it easier to use -- notably with modeling the resource type result. I'm just in a tough position where showing favoritism in one area would be unfair to other areas that could also be updated, which leads into prioritizing one use-case over another etc. So thank you for making that change.
I've made two suggested changes to enable default values in the requests. Default values are something we normally don't use either, but I think it's a fair compromise given the other changes you made.
Please let me know if you have any questions.
| ToResourceTypeListQuery() (string, error) | ||
| } | ||
|
|
||
| // ListOpts allows the filtering of collections through the API. |
There was a problem hiding this comment.
We can modify this a little to still use a default value but allowing a user to opt-out:
type ListOpts struct {
NameRegex string `q:"name"`
SupportStatus SupportStatus `q:"support_status"`
WithDescription *bool `q:"with_description"`
}
func (opts ListOpts) ToResourceTypeListQuery() (string, error) {
if opts.WithDescription == nil {
iTrue := true
opts.WithDescription = &iTrue
}
q, err := gophercloud.BuildQueryString(opts)
return q.String(), err
}This will cause with_description to be set to true by default, but allowing someone to opt-out and set it to false without having to override the method.
There was a problem hiding this comment.
You know, if we have to include this option then with this output format I think it's fine to just have it default to false and not try to do anything clever.
There was a problem hiding this comment.
Right, but I thought the idea was to have with_description set to true by default so the user receives the more detailed API response?
There was a problem hiding this comment.
Originally I was looking through the API code and interpreted it to mean that the old format was there only to support older versions of the client. But on closer inspection, it means that not specifying an option is there only to support older versions. The current openstackclient CLI still defaults to false. And with this data structure as the output it seems less weird to leave the description blank if the caller doesn't request it. So I'm ok with defaulting to false.
| ) | ||
|
|
||
| // GenerateTemplateOpts allows the filtering of collections through the API. | ||
| type GenerateTemplateOpts struct { |
There was a problem hiding this comment.
Similar here:
type GenerateTemplateOpts struct {
TemplateType *GeneratedTemplateType `q:"template_type"`
}
func (opts GenerateTemplateOpts) ToGenerateTemplateQuery() (string, error) {
if opts.TemplateType == nil {
templateType := TemplateTypeHOT
opts.TemplateType = &templateType
}
q, err := gophercloud.BuildQueryString(opts)
return q.String(), err
}
func GenerateTemplate(client *gophercloud.ServiceClient, resourceType string, opts GenerateTemplateOptsBuilder) (r TemplateResult) {
url := generateTemplateURL(client, resourceType)
if opts == nil {
opts = GenerateTemplateOpts{}
}
query, err := opts.ToGenerateTemplateQuery()
if err != nil {
r.Err = err
return
}
url += query
_, r.Err = client.Get(url, &r.Body, nil)
return
}There was a problem hiding this comment.
It'll just default to the empty string, so I don't think we even need to do pointers.
| } | ||
|
|
||
| // PropertyType represents the expected type of a property or attribute value. | ||
| type PropertyType string |
There was a problem hiding this comment.
and also to have constants already defined to use in switch statements
Good point
Add client support for listing available resource types in Heat, getting their schemata, and downloading example templates for use as provider templates. Fixes gophercloud#1805 Signed-off-by: Zane Bitter <zbitter@redhat.com>
|
Build failed.
|
|
Not sure why the Ironic tests have started failing now, but #1812 fixes them. |
|
This is ready to go from my perspective. Thanks for the detailed reviews, they were very helpful. |
Add client support for listing available resource types in Heat, getting
their schemata, and downloading example templates for use as provider
templates.
For #1805
Links to the line numbers/files in the OpenStack source code that support the
code in this PR: