Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2231)

Issue 6875052: Add filtering to ListServers APIs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 3 months ago by wallyworld
Modified:
11 years, 3 months ago
Reviewers:
mp+137766
Visibility:
Public.

Description

Add filtering to ListServers APIs The results of the nova APIs ListServers and ListServerDetails can now be filtered by status, name etc. https://code.launchpad.net/~wallyworld/goose/filter-servers-list/+merge/137766 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 10

Patch Set 2 : Add filtering to ListServers APIs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -13 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M http/client.go View 1 chunk +1 line, -1 line 0 comments Download
M nova/nova.go View 1 5 chunks +54 lines, -4 lines 0 comments Download
M nova/nova_test.go View 1 6 chunks +54 lines, -8 lines 0 comments Download

Messages

Total messages: 3
wallyworld
Please take a look.
11 years, 3 months ago (2012-12-04 07:41:15 UTC) #1
jameinel
LGTM with some tweaks. https://codereview.appspot.com/6875052/diff/1/http/client.go File http/client.go (right): https://codereview.appspot.com/6875052/diff/1/http/client.go#newcode38 http/client.go:38: Params *url.Values I'm curious why ...
11 years, 3 months ago (2012-12-04 13:01:45 UTC) #2
wallyworld
11 years, 3 months ago (2012-12-05 00:46:41 UTC) #3
*** Submitted:

Add filtering to ListServers APIs

The results of the nova APIs ListServers and ListServerDetails can now be
filtered by status, name etc.

R=jameinel
CC=
https://codereview.appspot.com/6875052

https://codereview.appspot.com/6875052/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/6875052/diff/1/http/client.go#newcode38
http/client.go:38: Params         *url.Values
On 2012/12/04 13:01:45, jameinel wrote:
> I'm curious why this change?

Filter uses an embedded url.Values type. The Params attribute is passed by using
the expression filter.Values. For situations when ListServers and
ListServerDetails are passed a nil filter, the Params attribute needs to be a
pointer to avoid a nil dereferencing error.

https://codereview.appspot.com/6875052/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/6875052/diff/1/nova/nova.go#newcode24
nova/nova.go:24: const (
On 2012/12/04 13:01:45, jameinel wrote:
> I wonder if these should be typed as well as grouped as strings.
> type Status string
> StatusActive = Status("ACTIVE")
> and such.
> I don't know if we win much yet, but something to consider.

I considered it but then filter.Add(key, value) would need to cast value to
string each time and it didn't seem worth it. Filter uses an embedded url.Values
type and the Add() method comes from there. It takes string arguments. Also,
Add() is used for other filter keys like 'name' etc so it seems easier just to
keep everything as string.

https://codereview.appspot.com/6875052/diff/1/nova/nova.go#newcode71
nova/nova.go:71: //     resp, err := nova.ListServers(filter)
On 2012/12/04 13:01:45, jameinel wrote:
> Maybe it wouldn't help, as we don't have "filter.AddStatus()" we just have
> filter.Add(). Should we have typed Add?

Add is used for all sorts of filter keys: status, name etc. We could have
AddStatus(), AddName() etc for each, and these methods would call Add() after
casting the value to a string if required (eg name would be passed as a string
directly, status as a const). But I'm not sure it's worth it? Having said that,
the code comment above should have been "filter.Add(nova.FilterStatus,
nova.StatusActive)" which I will fix.

https://codereview.appspot.com/6875052/diff/1/nova/nova_test.go
File nova/nova_test.go (left):

https://codereview.appspot.com/6875052/diff/1/nova/nova_test.go#oldcode147
nova/nova_test.go:147: 
On 2012/12/04 13:01:45, jameinel wrote:
> There are a fair number of rename-the-instance-variable changes here. It might
> be nice to have mechanical changes like this factored into a separate branch
so
> that it doesn't confuse the actual diff. Obviously at some threshold because
it
> takes your effort to do the split out.
> 
> This patch had *just enough* things I needed to think about, that adding the
> things-that-are-mechanical made it harder.

Ah sorry, I did these as a drive by to fix some naming consistency issues.

https://codereview.appspot.com/6875052/diff/1/nova/nova_test.go
File nova/nova_test.go (right):

https://codereview.appspot.com/6875052/diff/1/nova/nova_test.go#newcode53
nova/nova_test.go:53: }
On 2012/12/04 13:01:45, jameinel wrote:
> is s.testServer field out if SetUp fails?
> Otherwise it seems you might have a nil object in s.testServer but you aren't
> testing for it. eg
>  if s.testServer != nil {}
> 
> Certainly SetUpSuite could fail because your credentials weren't set, and that
> shouldn't cause a panic because testServer is nil.
> (I don't know if GoCheck always runs TearDown even when SetUp fails, but it
> should)

Fair point, fixed.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b