> Hi Brian,
> 
> Once again, great work. Couple little things:
> 
> 1) Something I did not think about for the results filtering MP, as well as
> this one, is that we will want to bump the minor version since we are adding
> functionality to the API in these two MPs.
> 
> You can do this in a separate merge proposal, I suppose. Basically, what needs
> to happen is that we need a way of describing the 1.1 API (1.0 + filters and
> limits) in the /versions endpoint.
> 
> The reason this is important is that the API result for /images has changed:
> we go from returning ALL public images in no particular order to returning
> SOME public images in a particular order (by created_at DESC). So, we should
> bump the minor version of the API to indicate to clients that this has been
> done.
> 
> Since we are in the middle of the D1 milestone, I'm not too concerned about
> having the results-filter branch, then this branch, then the API 1.1 branch
> going into trunk in that particular order. I'll handle the versioning stuff
> next week when I return from vacation.

According to the last versioning-related discussion we had, these changes were going to be part of v1.0. We haven't cut an official v1.0 release yet, and the limiting changes are definitely a major version change. I say we let the filtering and pagination branches in then call it v1.0. Any new feature (sets?) bump the version.

> 2) I'd prefer to set the default limit to < 1000. I'm thinking 25 or 50 would
> be a more appropriate default limit. Also, please make this a constant
> somewhere, not just a number in the method signature...

I meant to do that before I pushed the branch. Now it's a constant in registry/server.py, set to 25

> 3) Unless I'm mistaken these functions:
> 
> 50      +def limit_collection(items, marker, limit):
> 243     + def _limit_collection(self, req, images):
> 
> in glance.common.wsgi and glance.registry.server, respectively, can be
> removed. You've pushed (correctly) the limiting and offset down into the
> registry db query.

Good catch. Removed them.
 
> 4) This:
> 
> 114     if filters != None:
> 115     - action = "/images?%s" % urllib.urlencode(filters)
> 116     + params = filters
> 117     else:
> 118     - action = "/images"
> 119     + params = {}
> 
> Might be better as:
> 
> params = filters or {}

Yep. Did this in a bunch of places.

> 5)
> 
> 184     + order_by(models.Image.id)
> 
> I think I would prefer:
> 
> order_by(desc(models.Image.created_at))

Done. I kept the secondary order_by id for predictability in the behavior of marker b/c created_at is not unique.

Shoot for custom ordering in a future milestone (for v1.1)?

> 6)
> 
> 192     + if marker != None:
> 193     + query = query.filter(models.Image.id > marker)
> 
> Should be:
> 
> if marker != None:
>     query = query.offset(marker)
> 
> That way, SQLAlchemy will pass the marker as-is to the query as a LIMIT X
> OFFSET Y. This allows more than just the Image.id column to be used for
> ordering, and will allow you to add custom ordering to the API ;)

Actually, the term "offset" means something different than "marker". Offset is an index in a list of images, while marker is the actual id of an image in that list. I did have to change the code to support the new created_at sort, but I kept the original functionality.