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
adds dns support for ns1 provider #710
Conversation
Thanks @jetbird for the PR. Let me know when this is completed. Also if you install flake8 locally you can run the code checks to pass the failing lint build job in travis. |
@tonybaloney @Kami I think we have a problem in implementing the def create_record(self, name, zone, type, data, extra=None) method. From what I understand when reading the API, the provider does not support adding a name for the new record. Under Records take a look on the PUT method https://ns1.com/api/ |
@jetbird I checked the docs and it looks like you need to provide a fully qualified record name when creating a record. This means record name + domain name. It looks like that in the docs they refer to the record name + domainname as "domain". For example if record name is |
@Kami, you are right. Coding it now. |
@tonybaloney It is finished man. Please take a look at the code. |
@jetbird the lint is failing: |
@tonybaloney I managed to flake8 the code. |
@jetbird Nice work so far 👍 The next step would be adding some tests and documentation :) |
common_attr = ['zone', 'id', 'type'] | ||
extra = {} | ||
for key in item: | ||
if key not in common_attr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be if key in common_attr
(dropping not
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a key is not in common_attr list then we add it in the extra dict as a key with the item.get('key') as value. The keys in common_attr are needed to build the Zone object. Why do you think we should drop it?
@Kami @tonybaloney guys did you find some time to check the tests? |
|
||
from libcloud.test import MockHttp | ||
from libcloud.test.file_fixtures import DNSFileFixtures | ||
from libcloud.test.secrets import DNS_PARAMS_NSONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not exist, you need to update test/secrets-dist.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonybaloney I thought DNS_PARAMS_NSONE is supposed to stay locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonybaloney I updated and pushed secrets.py. I do not know why the travis builds are failing. The tests pass when run on my local machine.
@jetbird secrets.py is on the ignore list, part of the build is to copy secrets-dist.py to secrets.py. You need to apply your changes to secrets-dist and apply those changes. Not secrets.py |
@tonybaloney Done. |
Excellent, thats the one. If CI gives the 🍏 then I will merge. LGTM |
@tonybaloney It does not look like that. I also updated the fixtures with blank lines at each one of them, but when pushing on github they get ignored. |
@jetbird Do you have Tox? This would really help you testing before you commit. |
@tonybaloney I will install it now. |
@tonybaloney I installed tox and ran the tests with sudo tox, but the problem is that many interpreters are not found. I am not familiar with tox, so I guess I need to read the docs. Where should I install these interpreters? Is the following the only problem or are there more. Because if it is the only one I can fix this and take care later about the tox? except NsOneException, e: is not a valid syntax in Python 3. |
@jetbird you can review the errors here https://travis-ci.org/apache/libcloud/builds/116256818 |
@tonybaloney I decided to follow this approach to keep one code base through different versions of python.
Now will try to pass the tests with tox. |
Use |
@tonybaloney I get the following output when running the command.
|
@tonybaloney I managed to run the tests against python3.4 and they pass. The only check that is failing on travis is the lint check. Maybe it is the fixtures, I have no idea. Thanks for being so patient and helping. |
@Kami @tonybaloney Hey guys when using flake8 on nsone module and test_nsone I do not get any errors. Why do you think the lint build job in travis is failing? |
@tonybaloney @Kami any update on this please? |
from libcloud.dns.drivers.nsone import NsOneDNSDriver | ||
from libcloud.utils.py3 import httplib | ||
from libcloud.dns.types import ZoneDoesNotExistError, ZoneAlreadyExistsError,\ | ||
RecordDoesNotExistError, RecordType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 spaces after the comma. this is failing the lint. remove one of the spaces and just commit that change then we're good to go @jetbird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok man :) 👍
@tonybaloney I removed the extra spaces, but it still fails. |
@jetbird so close!! libcloud/test/dns/test_nsone.py:10:2: E121 continuation line under-indented for hanging indent |
yeah!! |
@tonybaloney finally 👯 |
Merged. Thanks @jetbird it might be time to get a Python editor with a built in linter! Try Komodo or Atom :-) |
@tonybaloney Thanks for the help and support 👍 I am using pycharm and flake8 is not working ok. I installed pylint via pip. I will be contributing again, so please if you can give me your email address would be great :) |
The methods that deal with records are not finished yet.