Skip to content
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

Replace RunAbove driver by OVH #891

Closed
wants to merge 4 commits into from
Closed

Replace RunAbove driver by OVH #891

wants to merge 4 commits into from

Conversation

ZuluPro
Copy link
Contributor

@ZuluPro ZuluPro commented Oct 5, 2016

Replace RunAbove driver by OVH

Description

RunAbove is the OVH's test platform. Now its compute is closed, I replaced by their final solution at OVH.

Status

Done

Checklist (tick everything that applies)

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

Copy link
Contributor

@tonybaloney tonybaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much @ZuluPro I've made some comments but overall this PR is excellent.

@@ -115,17 +116,23 @@ def request_consumer_key(self, user_id):

def get_timestamp(self):
if not self._timedelta:
url = 'https://%s/%s/auth/time' % (API_HOST, API_ROOT)
url = 'https://%s%s/auth/time' % (API_HOST, API_ROOT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just clarify for me that it was intentional to drop / in between %s and %s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because API_ROOT already starts with '/' (https://github.com/apache/libcloud/pull/891/files/a5ed7e1f907cd1fcb5dde3f873bf4855012f3fe7#diff-b98d1474f4f20fc646a3ac82fcbd0387R37) . So it will make a doublet.
Maybe it worked with RunAbove, but not with OVH

self.connection.request(action, method='DELETE')
return True

def list_sizes(self, location=None):
action = API_ROOT + '/flavor'
data = {}
action = '%s/cloud/project/%s/flavor' % (API_ROOT, self.project_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You make this transformation a lot. I would either store this as an instance level attribute or have a private method for _request_for_project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactor code with _get_project_action.

Provider.RUNABOVE:
('libcloud.compute.drivers.runabove', 'RunAboveNodeDriver'),
Provider.OVH:
('libcloud.compute.drivers.ovh', 'OvhNodeDriver'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provider.RUNABOVE should be added to the deprecated dictionary with a message about using the OVH service instead of RA so that when someone runs get_driver(Provider.RUNABOVE) they get a deprecated driver exception

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Say me if I miss something!

@tonybaloney
Copy link
Contributor

@ZuluPro OK. good to merge. great work. thanks

@asfgit asfgit closed this in cc9a013 Oct 6, 2016
asfgit pushed a commit that referenced this pull request Oct 6, 2016
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.

None yet

2 participants