[WIP] qos: list rule types#1030
Conversation
|
Build succeeded.
|
|
@Twister915 Thanks for working on this. Let me know when this PR is ready for a review :)
It might be beneficial to do a few PRs individually in order to get familiar with certain coding styles. This might help reduce multiple changes across PRs. For example, this PR is really good. There are a few areas that need touched up, though. I'm more than happy to point them out but only if it's ready :) |
|
@jtopjian thanks! happy to help 👍 I was planning on implementing each route in a different PR, because that's what the contributor guide said to do. Is that what you mean by doing them individually? Would you rather me put them in one PR after I get the hang of it? Also... I'm pretty sure I'm done with this specific route, so feel free to review it before I start submitting further PRs for other routes. I have [WIP] in the title because I assume I wouldn't want to merge this route without merging the other ones. |
Yes, this is correct.
Still one PR per route/call, please :)
This. It's totally fine to merge individual routes/calls without having the entire set ready, especially in the beginning and for new contributors. This helps both the contributor and reviewer get a better feel of the resource. There's usually a good amount of changes done in the beginning but things settle down after a few PRs and then you can easily open multiple PRs without concern of doing a large chain of rebasing. |
jtopjian
left a comment
There was a problem hiding this comment.
@Twister915 Please let me know if you have any questions.
This is a really good PR. I realize I've suggested rewriting most of the implementation. There was not anything wrong with the way you wrote it -- in fact, I spent some time debating which was the better way. But I'm leaning on how all of the other "List" calls are implemented in Gophercloud here. To help with the rewrite, I've added most of the required code. Do let me know if you run into any problems, though.
Other areas of this PR (acceptance tests, unit tests, fixtures, etc) were all spot-on, too. Really nice work. Note that the tests will need to be changed too, though.
As well, please let me know if you have any questions.
| @@ -0,0 +1,4 @@ | |||
| /* | |||
| Package qos contains functionality for working with Neutron 'quality of service' resources. | |||
There was a problem hiding this comment.
nit: It'd be helpful to have an example of this call here. doc.go turns into the official documentation generated at godoc.org.
| /* | ||
| Package qos contains functionality for working with Neutron 'quality of service' resources. | ||
| */ | ||
| package qos |
There was a problem hiding this comment.
According to https://developer.openstack.org/api-ref/network/v2/#quality-of-service, it looks like QoS has "rule types" and "policies".
It might be a good idea to create subpackages for all of these:
networking/v2/extensions/qos/ruletypes
networking/v2/extensions/qos/policies
I'm not familiar with QoS and how ruletypes and policies work together, so if you feel everything could go under qos, I'm OK with that.
| } | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
I've debated this one for almost an hour. I understand that the results of retrieving the details of a rule type are very different than the list request and that the list request is only returning a Type field, but I think we should create a proper struct for RuleType, even though it'll only contain one field.
The more detailed result can be called RuleTypeDetails. Or choose names you feel are suitable.
I realize that this basically rewrites this entire file and I apologize about that. But I have a feeling this is better in the long-run. To help get you started, the contents of the file would look something like:
type RuleType struct {
Type string `json:"type"`
}
// The result of listing the qos rule types
type ListRuleTypesPage struct {
pagination.SinglePageBase
}
// IsEmpty indicates whether a RuleTypesPage is empty.
func (r ListRuleTypesPage) IsEmpty() (bool, error) {
v, err := ExtractRuleTypes(r)
return len(v) == 0, err
}
func ExtractRuleTypes(r pagination.Page) ([]RuleType, error) {
var s struct {
RuleTypes []RuleType `json:"rule_types"`
}
err := (r.(ListRuleTypesPage)).ExtractInto(&s)
return s.RuleTypes, err
}| func ListRuleTypes(c *gophercloud.ServiceClient) (result ListRuleTypesResult) { | ||
| _, result.Err = c.Get(listRuleTypesURL(c), &result.Body, nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
With the changes made to results.go, this function will now look something like:
// ListRuleTypes retrieves the QoS rule types.
func ListRuleTypes(client *gophercloud.ServiceClient) pagination.Pager {
url := listRuleTypesURL(c)
return pagination.NewPager(client, url, func(r pagination.PageResult) pagination.Page {
return ListRuleTypesPage{pagination.SinglePageBase(r)}
})
}If you were to put this in a qos/ruletypes package, the function name could be changed to just List.
| } | ||
| ] | ||
| }` | ||
| ) |
There was a problem hiding this comment.
nit:
const ListRuleTypesResponse = `
{
"rule_types": [
...
}
`…put the list into a struct, use pagination, add docs, and update tests to work with the new code)
|
Thanks for the thorough code-review! I've implemented the changes requested, and agree with you on moving this set of calls into a |
|
@jtopjian When submitting my next pull request, should I base it off this branch? Also, should we merge this to master before merging the next pull request? The diff between master and the pull request I want to make includes all the changes from this pull request, making it difficult to review (presumably). |
|
Build succeeded.
|
Totally your preference. I prefer the "chaining method" where the next PR's branch will be based off of this branch in anticipation of this PR being merged. Some people have based all PRs off of a common master. This causes duplication between PRs, but it's a style they prefer.
Yes, I'll have some time for another review later today or tomorrow. If all looks good, I'll merge it. When chaining PRs, you have to factor in a parent PR needing changes. Then you have to rebase all the way down the chain. This is why I recommended not opening too many PRs at first :) once you get a feel for the style and such it's easier to have multiple PRs open without worry of changes required. If your PRs do not require any changes and you're chaining branches from one to another, Github will automatically recognize this and cleanly merge one after another even though child PRs show multiple commits. It's a little confusing -- let me know if you have any questions :) |
jtopjian
left a comment
There was a problem hiding this comment.
@Twister915 Really nice :)
For #1027, Implements listing rule types (
GET /rule-types)Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
https://github.com/openstack/neutron/blob/master/neutron/objects/qos/rule_type.py#L61
WIP, implementing other routes