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

[LIBCLOUD-691] Add Onapp IaaS platform support #502

Conversation

MatthiasWiesnerCC
Copy link

@MatthiasWiesnerCC MatthiasWiesnerCC force-pushed the LIBCLOUD-691_onapp_support branch 2 times, most recently from 6f27333 to 507fbaa Compare April 9, 2015 18:02
@Kami
Copy link
Member

Kami commented Apr 11, 2015

Great, thanks.

I will review it and add the comments inline.

name = 'OnApp'
website = 'http://onapp.com/'

def create_node(self, label, memory, cpus, cpu_shares, hostname,
Copy link
Member

Choose a reason for hiding this comment

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

This method is part of the standard API so all the arguments which are not part of standard API (looks like all of them :)) should have an ex_ prefix.

@Kami
Copy link
Member

Kami commented May 3, 2015

@MatthiasWiesnerCC bump^^

@TooAngel TooAngel force-pushed the LIBCLOUD-691_onapp_support branch from 507fbaa to 932420d Compare May 28, 2015 08:55
@TooAngel
Copy link
Contributor

I took over the onapp integration on our side. I prefixed the non standard parameter with ex_. Also simplified the implementation to have less onapp specific classes and renamed the parameters which can be mapped to libcloud standard parameters.

@TooAngel TooAngel force-pushed the LIBCLOUD-691_onapp_support branch 2 times, most recently from a9cee1f to ce4e1dd Compare May 28, 2015 09:04
@TooAngel TooAngel force-pushed the LIBCLOUD-691_onapp_support branch 3 times, most recently from ac8bbad to 5c54d41 Compare July 31, 2015 13:19
Implement missing support for Virtual Server in OnApp node driver,
as described in the OnApp API documentation:
https://docs.onapp.com/display/34API/Virtual+Servers
@TooAngel
Copy link
Contributor

@Kami bump^^ :-)

@asfgit asfgit closed this in 592c934 Aug 1, 2015
@Kami
Copy link
Member

Kami commented Aug 1, 2015

Sorry for the delay.

The changes looked OK (except a weird test failure - see below), so I went ahead and merged it into trunk. Thanks.

There was a just a weird test failure which only happened on every Python version except 2.7.

FAIL: test_create_node (__main__.OnAppNodeTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "libcloud/test/compute/test_onapp.py", line 67, in test_create_node
    self.assertEqual('onapp-new-fred', node.name)
AssertionError: 'onapp-new-fred' != <MagicMock name='mock.object.__getitem__().__getitem__()' id='139971643315264'>

It turned out the failure was related to using is to compare string values in the test (b3ca86b). The is operator compares the object pointers and not the values so it should only be used to check for values such as True and None.

The test passing on 2.7 was just a pure luck :)


class OnAppNodeTestCase(LibcloudTestCase):
def setUp(self):
def _request(*args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Also, just a quick heads up for the future - we have a framework in place for mocking HTTP responses (pretty much every driver tests use it).

So in the future, please use follow the approach other tests use.

@TooAngel TooAngel deleted the LIBCLOUD-691_onapp_support branch August 5, 2015 08:42
@TooAngel
Copy link
Contributor

TooAngel commented Aug 5, 2015

Thank you for merging.

I also changed the tests to the libcloud test framework (fec2964). Should I open a separate issue for that, send the pull request (#560), ...?

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.

3 participants