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

loadbalancer/rackspace/list_balancers: add params #803

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
2 participants
@raittes
Contributor

raittes commented Jun 6, 2016

Load Balancer / Rackspace: allow params on listing load balancers

Description

Allow to set params to be submitted in the query string to API request.
By default, Rackspace API return a maximum of 100 items at a time.
This change allow to set "offset" in the params, to paginate requests.
Ex. list_balancers(ex_params={'offset': 100})

Status

  • done, ready for review

Checklist

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jun 6, 2016

Contributor

thanks for the contribution @raittes - libcloud convention is that any arguments which are not present in the base class methods are prepending with ex_.
Please could you rename the parameter to ex_params.

Ideally, there would be named keyword arguments for typical arguments, according to the docs https://developer.rackspace.com/docs/cloud-load-balancers/v1/developer-guide/#list-load-balancers they would include status, nodeaddress and changes-since.
With a simple dictionary param, it's leaving it to the developer consuming to have to read the API documentation.

Contributor

tonybaloney commented Jun 6, 2016

thanks for the contribution @raittes - libcloud convention is that any arguments which are not present in the base class methods are prepending with ex_.
Please could you rename the parameter to ex_params.

Ideally, there would be named keyword arguments for typical arguments, according to the docs https://developer.rackspace.com/docs/cloud-load-balancers/v1/developer-guide/#list-load-balancers they would include status, nodeaddress and changes-since.
With a simple dictionary param, it's leaving it to the developer consuming to have to read the API documentation.

@raittes

This comment has been minimized.

Show comment
Hide comment
@raittes

raittes Jun 7, 2016

Contributor

Hi @tonybaloney, I've updated the PR, please check it out!
I've included the params for /loadbalancers, but also keep the ex_params, to allow setting general api params like limit and offset.
Thx!

Contributor

raittes commented Jun 7, 2016

Hi @tonybaloney, I've updated the PR, please check it out!
I've included the params for /loadbalancers, but also keep the ex_params, to allow setting general api params like limit and offset.
Thx!

@raittes

This comment has been minimized.

Show comment
Hide comment
@raittes

raittes Jun 16, 2016

Contributor

@tonybaloney, something is missing?

Contributor

raittes commented Jun 16, 2016

@tonybaloney, something is missing?

@raittes raittes changed the title from loadbalancer-rackspace: allow params to be set to loadbalancer/rackspace/list_balancers: add params Jun 16, 2016

Show outdated Hide outdated libcloud/loadbalancer/drivers/rackspace.py
if ex_changes_since:
params['changes-since'] = ex_changes_since
for key, value in ex_params.iteritems():

This comment has been minimized.

@tonybaloney

tonybaloney Jun 17, 2016

Contributor

dict.iteritems() is not valid in Python 3, you must use items().

Which probably highlights that this code doesn't have a supporting unit test as TravisCI should have picked that up. please can you add a test.

@tonybaloney

tonybaloney Jun 17, 2016

Contributor

dict.iteritems() is not valid in Python 3, you must use items().

Which probably highlights that this code doesn't have a supporting unit test as TravisCI should have picked that up. please can you add a test.

This comment has been minimized.

@raittes

raittes Jun 28, 2016

Contributor

Fixed!

Tests are ok, I forced push and TravisCI may have missed something.
Here another build: https://travis-ci.org/raittes/libcloud/jobs/140894397

======================================================================
ERROR: test_destroy_balancer (libcloud.test.loadbalancer.test_rackspace.RackspaceLBTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/raittes/projects/raittes/libcloud/libcloud/test/loadbalancer/test_rackspace.py", line 187, in test_destroy_balancer
    balancer = self.driver.list_balancers()[0]
  File "/home/raittes/projects/raittes/libcloud/libcloud/loadbalancer/drivers/rackspace.py", line 385, in list_balancers
    for key, value in ex_params.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
@raittes

raittes Jun 28, 2016

Contributor

Fixed!

Tests are ok, I forced push and TravisCI may have missed something.
Here another build: https://travis-ci.org/raittes/libcloud/jobs/140894397

======================================================================
ERROR: test_destroy_balancer (libcloud.test.loadbalancer.test_rackspace.RackspaceLBTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/raittes/projects/raittes/libcloud/libcloud/test/loadbalancer/test_rackspace.py", line 187, in test_destroy_balancer
    balancer = self.driver.list_balancers()[0]
  File "/home/raittes/projects/raittes/libcloud/libcloud/loadbalancer/drivers/rackspace.py", line 385, in list_balancers
    for key, value in ex_params.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jun 28, 2016

Contributor

the commit came in after your comment, but the build passed now that the py3 compatibility issue was fixed up

Contributor

tonybaloney commented Jun 28, 2016

the commit came in after your comment, but the build passed now that the py3 compatibility issue was fixed up

@asfgit asfgit closed this in d1bcc70 Jun 28, 2016

asfgit pushed a commit that referenced this pull request Jun 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment