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

Maxihost provider #1298

Merged
merged 17 commits into from Jun 28, 2019
Merged

Maxihost provider #1298

merged 17 commits into from Jun 28, 2019

Conversation

mpempekos
Copy link
Contributor

@mpempekos mpempekos commented Jun 10, 2019

Implementation for connection and main actions for Maxihost provider

Description

Implemented main classes and methods for connecting with Maxihost's API. API ref can be found: https://developers.maxihost.com

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)

@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #1298 into trunk will decrease coverage by <.01%.
The diff coverage is 84.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1298      +/-   ##
==========================================
- Coverage   85.95%   85.95%   -0.01%     
==========================================
  Files         359      362       +3     
  Lines       73914    74090     +176     
  Branches     6705     6719      +14     
==========================================
+ Hits        63533    63681     +148     
- Misses       7699     7722      +23     
- Partials     2682     2687       +5
Impacted Files Coverage Δ
libcloud/compute/providers.py 85.71% <ø> (ø) ⬆️
libcloud/compute/types.py 100% <100%> (ø) ⬆️
libcloud/test/compute/test_maxihost.py 100% <100%> (ø)
libcloud/common/maxihost.py 61.9% <61.9%> (ø)
libcloud/compute/drivers/maxihost.py 80.43% <80.43%> (ø)
libcloud/test/compute/test_upcloud.py 90.06% <0%> (-1.33%) ⬇️
libcloud/common/openstack_identity.py 75.89% <0%> (-0.11%) ⬇️
libcloud/test/common/test_openstack_identity.py 98.06% <0%> (ø) ⬆️
libcloud/compute/drivers/openstack.py 85.24% <0%> (ø) ⬆️
libcloud/common/azure.py 84% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6540c12...8934ee9. Read the comment docs.

@Kami
Copy link
Member

Kami commented Jun 22, 2019

Thanks for the contribution 👍

It looks like a good start.

Since it's a bigger contribution, can you please also sign an ICLA (http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes)?

In addition to that, can you also add some more test cases for various edge cases (invalid credentials, etc.) and corresponding documentation page?

attr = {'hostname': name, 'plan': size.id,
'operating_system': image.id,
'facility': location.id.lower(), 'billing_cycle': 'monthly',
'ex_ssh_key_ids': ex_ssh_key_ids}
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the docs (https://developers.maxihost.com/reference#post_devices), the attribute name on the API side should be ssh_keys, right?

driver=self, extra=extra)
return node

def list_locations(self, available=True):
Copy link
Member

Choose a reason for hiding this comment

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

available -> ex_available.

data = self.connection.request('/account/keys')
return list(map(self._to_key_pair, data.object['ssh_keys']))

def create_key_pair(self, name, public_key=''):
Copy link
Member

@Kami Kami Jun 28, 2019

Choose a reason for hiding this comment

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

Should remove default argument value if the argument is required (aka (self, name, public_key)).

@Kami
Copy link
Member

Kami commented Jun 28, 2019

Thanks for adding the docs, etc.

There are some more small issues left which I will address when merging this into master.

@asfgit asfgit merged commit 10e2e6c into apache:trunk Jun 28, 2019
@Kami
Copy link
Member

Kami commented Jun 28, 2019

I've made small changes as per PR review comments (3a9ef1f, 1170e9c, 2fe5304), fixed tests (baf956e) and merged this into trunk.

In the future, more test cases for various edge cases (invalid credentials, API returns an error, etc.) would also be great.

Thanks again for your contribution.

@mpempekos
Copy link
Contributor Author

@Kami thanks for addressing the issues

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

Successfully merging this pull request may close these issues.

None yet

4 participants