Skip to content

fix path for profitbricks list_snapshots() method#1153

Closed
thehunmonkgroup wants to merge 1 commit intoapache:trunkfrom
stirlab:fix-profitbricks-list-snapshots
Closed

fix path for profitbricks list_snapshots() method#1153
thehunmonkgroup wants to merge 1 commit intoapache:trunkfrom
stirlab:fix-profitbricks-list-snapshots

Conversation

@thehunmonkgroup
Copy link
Copy Markdown
Contributor

Fix path for profitbricks compute driver list_snapshots method

Description

The leading slash breaks the URL in this method. With it, I get:

Traceback (most recent call last):
  File "./profitbricks.py", line 38, in <module>
    snapshots = driver.list_snapshots()
  File "/usr/lib/python2.7/dist-packages/libcloud/compute/drivers/profitbricks.py", line 1176, in list_snapshots
    method='GET'
  File "/usr/lib/python2.7/dist-packages/libcloud/compute/drivers/profitbricks.py", line 143, in request
    raw=raw
  File "/usr/lib/python2.7/dist-packages/libcloud/common/base.py", line 871, in request
    response = responseCls(**kwargs)
  File "/usr/lib/python2.7/dist-packages/libcloud/common/base.py", line 180, in __init__
    headers=self.headers)
libcloud.common.exceptions.BaseHTTPError

Removing the slash makes the method work again, and it's in line with the format of the other API requests in the driver that omit the full URL.

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@pquentin
Copy link
Copy Markdown
Contributor

Thanks! Do you think you could add a test?

@thehunmonkgroup
Copy link
Copy Markdown
Contributor Author

I poked around the testing code, and wasn't really able to understand how y'all have it set up.

I see there is already a test in there for listing snapshots, but I'm guessing it's wrong or already failing, and I don't really understand how to fix it.

I'm sorry, I'm not willing to spend any more time on the learning curve.

@pquentin
Copy link
Copy Markdown
Contributor

Thank you for your honesty.

@thehunmonkgroup
Copy link
Copy Markdown
Contributor Author

sure thing, i hope my current contribution can make it into a fix.

@pquentin
Copy link
Copy Markdown
Contributor

We're currently lacking reviewers so no PR has been merged for more than a month. Hopefully this situation will change at some point.

@asfgit asfgit closed this in aaf1574 Jan 26, 2018
@pquentin
Copy link
Copy Markdown
Contributor

It turns out that it's not easy to add a test here, because the fixture will be the same with and without the slash. I decided to merge this as is in trunk. Thanks!

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.

2 participants