Conversation
- Add export() method to REST API and Zone class - Add unit test for zone export - Add simple usage example Allows customers to export zones in BIND format for backup/migration.
|
examples/zone-export.py
Outdated
| print(zone_file) | ||
|
|
||
| # save to a file | ||
| with open("example.com.zone", "w") as f: |
There was a problem hiding this comment.
| with open("example.com.zone", "w") as f: | |
| with open("example.com.txt", "w") as f: |
Usually zone files just have a .txt extension; and zone is a tld, so calling them .zone could be a bit confusing.
There was a problem hiding this comment.
Changed to example.com.txt in line 31.
| @@ -0,0 +1,25 @@ | |||
| # | |||
There was a problem hiding this comment.
Can you add more to the example? This example just assumes the other endpoints have been called.
It needs to initiate the export; then poll the status until it gets a complete status; then to download the file.
ns1/rest/zones.py
Outdated
| errback=errback, | ||
| ) | ||
|
|
||
| def export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def export(self, zone, callback=None, errback=None): | |
| def get_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to get_zonefile_export()
ns1/rest/zones.py
Outdated
| errback=errback, | ||
| ) | ||
|
|
||
| def initiate_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def initiate_export(self, zone, callback=None, errback=None): | |
| def initiate_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to initiate_zonefile_export()
ns1/rest/zones.py
Outdated
| errback=errback, | ||
| ) | ||
|
|
||
| def export_status(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def export_status(self, zone, callback=None, errback=None): | |
| def status_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to status_zonefile_export()
ns1/rest/zones.py
Outdated
|
|
||
| def export(self, zone, callback=None, errback=None): | ||
| """ | ||
| Export zone as BIND-compatible zone file. |
There was a problem hiding this comment.
Could probably say this downloads the zonefile.
ns1/zones.py
Outdated
| zone=self.zone, callback=callback, errback=errback, **kwargs | ||
| ) | ||
|
|
||
| def export(self, callback=None, errback=None): |
There was a problem hiding this comment.
Can you replace these with a single function that:
- Initialises the export
PUT - Polls the status, until "COMPLETE" or "FAILED" or some timeout hits.
- Calls/returns the download endpoint.
There was a problem hiding this comment.
Enhanced the example with detailed comments explaining the complete workflow. The export() method now handles all three steps automatically: initiate, poll status, and download.
ns1/zones.py
Outdated
| :return: zone file content as string | ||
| :raises ZoneException: if export fails or times out | ||
| """ | ||
| import time |
There was a problem hiding this comment.
You shouldn't import packages inside functions, can you move this to the top with the other imports?
ns1/zones.py
Outdated
| self.zone, callback=callback, errback=errback | ||
| ) | ||
|
|
||
| if callback: |
There was a problem hiding this comment.
Is this necessary here? you're already sending get_zonefile_export so any callback will get called above?
ns1/zones.py
Outdated
| import time | ||
|
|
||
| # Initiate the export | ||
| self._rest.initiate_zonefile_export(self.zone) |
There was a problem hiding this comment.
You should check the response here; make sure it's 200.
If something goes wrong here then we can't tell and it'll start polling for something that will never finish.
ns1/zones.py
Outdated
| status_response = self._rest.status_zonefile_export(self.zone) | ||
| status = status_response.get("status") | ||
|
|
||
| if status == "COMPLETE": |
There was a problem hiding this comment.
| if status == "COMPLETE": | |
| if status == "COMPLETED": |
examples/zone-export.py
Outdated
| f.write(zone_file) | ||
| print("Zone file saved to example.com.txt") | ||
|
|
||
| # Made with Bob |
There was a problem hiding this comment.
Before merging, let's confirm if this is something that should be included in public repositories. I suspect not, but let me check.
examples/zone-export.py
Outdated
| @@ -0,0 +1,35 @@ | |||
| # | |||
| # Copyright (c) 2025 NSONE, Inc. | |||
There was a problem hiding this comment.
As this is new code and yet to be released, I would suggest we add 2026 to the copyright? @ddevine-NS1 - any thoughts on this?
Not sure about the other pre-existing files.
There was a problem hiding this comment.
Yeah agree, we should update the copyright
ns1/rest/zones.py
Outdated
| """ | ||
| return self._make_request( | ||
| "GET", | ||
| f"{self.ROOT}/{zone}/export", |
There was a problem hiding this comment.
Is the use of {self.ROOT} here instead of export/zonefile in the two previous function definitions intentional?
There was a problem hiding this comment.
The new methods correctly use export/zonefile/{zone} paths as per the API spec.
ns1/rest/zones.py
Outdated
| # Note: This endpoint returns raw zone file text, not JSON | ||
| # The transport layer will try to parse it as JSON and fail | ||
| # We catch that exception and extract the raw body text | ||
| from ns1.rest.errors import ResourceException |
There was a problem hiding this comment.
imports need to go on the top of the file
There was a problem hiding this comment.
Moved from .errors import ResourceException at the top
ns1/rest/zones.py
Outdated
| # The body is the third argument in ResourceException | ||
| if hasattr(e, 'args') and len(e.args) >= 3: | ||
| body = e.args[2] | ||
| if callback: |
There was a problem hiding this comment.
Why is the callback being checked here?
There was a problem hiding this comment.
the callback check was unnecessary since _make_request() already handles callbacks. I've removed it. Now the method simply returns e.body directly when it's a valid zonefile response.
ns1/rest/zones.py
Outdated
| # If it's about invalid JSON, that's expected - extract the body | ||
| if "invalid json in response" in str(e): | ||
| # The body is the third argument in ResourceException | ||
| if hasattr(e, 'args') and len(e.args) >= 3: |
There was a problem hiding this comment.
ResourceException is a custom error, you can access the body like:
body = e.body
invalid json in response is too generic, since any http/txt response will probably result in this error too and this will just assume it's a zonefile.
Can you update this to check that the response code is valid otherwise raise the error.
And to check that the content type header is valid too.
examples/zone-export.py
Outdated
| # 1. Initiate the export job | ||
| # 2. Poll the status until complete or failed | ||
| # 3. Download and return the zone file content | ||
| zone = api.loadZone("example.com") |
There was a problem hiding this comment.
Can you set example.com to be a variable and then use that for the script, it's a bit neater for using it.
There was a problem hiding this comment.
Made the output filename dynamic using f"{zone_name}.txt"
hhellyer
left a comment
There was a problem hiding this comment.
Code looks good, I think there's one improvement/simplification you can make and we probably ought to get the copyright statement correct but no major issues!
hhellyer
left a comment
There was a problem hiding this comment.
LGTM - Thanks for making changes.
Allows customers to export zones in BIND format for backup/migration.