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

Clustering policytype list #860

Merged
merged 8 commits into from
Mar 30, 2018

Conversation

dkt26111
Copy link
Contributor

For #823

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

API doc:
https://developer.openstack.org/api-ref/clustering/#list-policy-types

API code:
https://github.com/openstack/senlin/blob/master/senlin/api/openstack/v1/policy_types.py#L30

@coveralls
Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage increased (+0.02%) to 73.9% when pulling 12518a2 on dkt26111:clustering_policytype_list into 2daf304 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 30, 2018

Build succeeded.

@dkt26111 dkt26111 changed the title [wip] Clustering policytype list Clustering policytype list Mar 30, 2018
@dkt26111
Copy link
Contributor Author

@jtopjian: Requesting code review. I'm working with JackKuei on implementing the senlin APIs.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@dkt26111 Thanks for this!

Overall this is really good. There are a handful of small nits that I wouldn't consider blocking (just things to clean up in the next PR).

However, one thing I'm curious about is if this is applicable here:

https://github.com/openstack/senlin/blob/c0ef921768046a8795aa1f231ec6fafd5e67f3ea/senlin/engine/service.py#L510

If so, then it looks like this API call supports options for listing (limit, marker, sort, etc). Can you verify if this is true (I'm setting up a Senlin and Zaqar environment to help you folks out with validating these PRs but I'm running blind at the moment).

If list options are a thing, we'll want to add proper support for ListOpts. A simple example of how to implement that is here:

// ListOpts filters the Tenants that are returned by the List call.
type ListOpts struct {
// Marker is the ID of the last Tenant on the previous page.
Marker string `q:"marker"`
// Limit specifies the page size.
Limit int `q:"limit"`
}
// List enumerates the Tenants to which the current token has access.
func List(client *gophercloud.ServiceClient, opts *ListOpts) pagination.Pager {
url := listURL(client)
if opts != nil {
q, err := gophercloud.BuildQueryString(opts)
if err != nil {
return pagination.Pager{Err: err}
}
url += q.String()
}
return pagination.NewPager(client, url, func(r pagination.PageResult) pagination.Page {
return TenantPage{pagination.LinkedPageBase{PageResult: r}}
})
}

// TenantPage is a single page of Tenant results.
type TenantPage struct {
pagination.LinkedPageBase
}
// IsEmpty determines whether or not a page of Tenants contains any results.
func (r TenantPage) IsEmpty() (bool, error) {
tenants, err := ExtractTenants(r)
return len(tenants) == 0, err
}
// NextPageURL extracts the "next" link from the tenants_links section of the result.
func (r TenantPage) NextPageURL() (string, error) {
var s struct {
Links []gophercloud.Link `json:"tenants_links"`
}
err := r.ExtractInto(&s)
if err != nil {
return "", err
}
return gophercloud.ExtractNextURL(s.Links)
}

SupportStatus map[string][]SupportStatusType `json:"support_status,omitempty"`
}

// SupportStatus represents the support status information for a clustering policy type
Copy link
Contributor

Choose a reason for hiding this comment

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

SupportStatusType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct. I will fix this.

import "github.com/gophercloud/gophercloud"

var apiVersion = "v1"
var apiName = "policy-types"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these could be constants:

const (
  apiVersion = "v1"
  apiName = "policy-types"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I'll fix it.

type PolicyType struct {
Name string `json:"name"`
Version string `json:"version"`
SupportStatus map[string][]SupportStatusType `json:"support_status,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: omitempty isn't really needed here - it's only useful in requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that too.

@jtopjian
Copy link
Contributor

@dkt26111 whoops - actually, my mistake. I was looking at policy list and not policy type list :)

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@dkt26111 Sorry about getting those two APIs mixed up. This looks good to me. Let me know if you want to clean up the nits now or later. If the latter, I can merge this now.

#824 will need to be rebased if this is merged (or vice-versa depending on if #824 is merged first)

@dkt26111
Copy link
Contributor Author

If everything looks good to you in the latest commits, you can go ahead and merge it. I will work with Jackkuei to rebase #824 .

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 30, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

@dkt26111 All looks good! Really nice work on this.

A note on microversions: we haven't figured out a good way of handling them in Gophercloud yet. Basic microversion stuff like what you're testing here is totally fine.

However if there will be times when, for example, a version 1.5 request or response body is different than a 1.0 request/body (meaning the struct fields are different and/or incompatible), this is the area we're still looking into. We can discuss different options if we get into that situation with Senlin.

@jtopjian jtopjian merged commit 781450b into gophercloud:master Mar 30, 2018
This was referenced Mar 30, 2018
problemv pushed a commit to problemv/gophercloud that referenced this pull request Apr 4, 2018
* Clustering PolicyType list implementation

* ran gofmt

* fix policytype list example

* add policytype list acceptance test

* remove admin required

* Fix review comments

* add acceptance test for microversion 1.5

* Fix wrong method name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants