Zone file import - support missing params. #126

hhellyer · 2024-06-21T14:22:33Z

This PR adds support for missing parameters when importing zone files:

networks
views
name

As both zone creation with or without a zone file to prepopulate it use the same call, just with or without the zonefile specificed, the "name" field also added to the api to create a zone and "views" is added to the list of fields passed through on zone creation.

hhellyer · 2024-06-25T07:44:58Z

Since this will need an update to the changelog before releasing I've added my proposed changelog update below. (It is a bit wordy because of the need to differentiate between behaviour changes when creating a zone and new things when creating a zone with a zonefile.)

## 0.20.1 (June ??th, 2024)

ENHANCEMENTS:
* Adds support for specifying a list of views when creating zones with or without a provided zone file.
* Adds support for specifying a zone name other than the FQDN when creating zones with or without a provided zone file.
* A specified list of networks for a zone was only applied to zone creation when a zone file was not provided. If a list of networks is specified it will now be applied when creating zones by uploading a zone file as well.

shane-ns1

There's a typo (zone instead of name in a docstring), but more important is that this changes order of parameters passed to the API.

shane-ns1 · 2024-06-25T12:31:02Z

ns1/__init__.py

@@ -371,7 +377,8 @@ def createZone(
        If zoneFile is specified, upload the specific zone definition file
        to populate the zone with.

-        :param str zone: zone name, like 'example.com'
+        :param str zone: zone FQDN, like 'example.com'
+        :param str zone: zone name override, name will be zone FQDN if omitted


Suggested change

:param str zone: zone name override, name will be zone FQDN if omitted

:param str name: zone name override, name will be zone FQDN if omitted

It is possible in Python to declare keyword-only arguments:

https://peps.python.org/pep-3102/

But we don't have that now, so order matters.

If you want to reorder the parameters like this you'll need to make a major release, as it can break the API. Alternately you can put it at the end. 😄

We're < 1.0 so we don't have to do a major release for such things (I think). But we will be breaking stuff!

shane-ns1 · 2024-06-25T12:31:44Z

ns1/rest/zones.py

    def _buildBody(self, zone, **kwargs):
        body = {}
        body["zone"] = zone
        self._buildStdBody(body, kwargs)
        return body

    def import_file(
-        self, zone, zoneFile, callback=None, errback=None, **kwargs
+        self, zone, zoneFile, name=None, callback=None, errback=None, **kwargs


Similar to the change in __init__.py, this reorders the parameters and is a breaking change.

shane-ns1 · 2024-06-25T12:34:16Z

ns1/rest/zones.py

+        # parameter but the zonefile API expects parameters containing comma
+        # seperated values.
+        if fields.get("networks") is not None:
+            networksStrs = map(str, fields["networks"])


I guess this is because people may pass networks as numbers?

Our API takes a list of network numbers.
Unfortunately it takes them in a single query parameter of the form networks=1,2,3 which is not how you should pass an array via parameters, as I've found out while working on the other side of our import API, and not how the request library will pass an array. So we need to convert the array to a string.
If allowed to the libs will pass an array using multiple params of the same name:
networks=1&networks=2&networks=3
but our API won't accept that.

ns1/rest/zones.py

shane-ns1 · 2024-06-25T12:35:06Z

ns1/rest/zones.py

+            params["name"] = name
+        return params
+
+    def create(self, zone, name=None, callback=None, errback=None, **kwargs):


Here also, changing the order of parameters may break things for some users.

shane-ns1 · 2024-06-25T12:35:35Z

ns1/zones.py

@@ -90,14 +90,21 @@ def success(result, *args):
            self.zone, callback=success, errback=errback, **kwargs
        )

-    def create(self, zoneFile=None, callback=None, errback=None, **kwargs):
+    def create(
+        self, name=None, zoneFile=None, callback=None, errback=None, **kwargs


This may break the API for folks.

cbertinato · 2024-06-25T13:06:24Z

ns1/rest/zones.py

+        # parameter but the zonefile API expects parameters containing comma
+        # seperated values.
+        if fields.get("networks") is not None:
+            networksStrs = map(str, fields["networks"])


Snake case in Python.

cbertinato · 2024-06-25T13:07:19Z

ns1/rest/zones.py

        return self._make_request(
            "PUT",
-            "%s/%s" % (self.ROOT, zone),
+            "%s/%s" % (self.ROOT, name),


Use f-strings. I know you're just changing a variable name, but might as well update while we're at it.

Done, on this function, create(), and on import(). I've left the others in this file as those were the two I altered and (more importantly) wrote tests for. There aren't specific tests for the other functions in this file.

cbertinato · 2024-06-25T13:07:47Z

tests/unit/test_zone.py

+    if name is not None:
+        params["name"] = name
+
+    zoneFilePath = "{}/../../examples/importzone.db".format(


Remaining comment, but also a nitpick. So change it if you'd like, but I won't block on it.

Add support for missing parameters when importing zone files: - networks - views - name Support for the "name" field also added to create without zonefile.

ns1/rest/zones.py

Use f-strings and be more pythonic... Co-authored-by: Shane Kerr <133137948+shane-ns1@users.noreply.github.com>

shane-ns1 requested changes Jun 25, 2024

View reviewed changes

cbertinato reviewed Jun 25, 2024

View reviewed changes

cbertinato requested changes Jun 25, 2024

View reviewed changes

hhellyer added 7 commits October 28, 2024 13:52

Zone file import - support missing params.

45de463

Add support for missing parameters when importing zone files: - networks - views - name Support for the "name" field also added to create without zonefile.

Fix path to sample zone file.

ea88715

Fix linter errors, improve docs.

5b5cdfa

Fix linter line length errors.

df9c177

Fix review comments.

ec64b2e

Use f-strings for formatting.

cd068b4

Update version and changelog for release.

f9e89a9

hhellyer force-pushed the import_to_networks_views branch from 4728a24 to f9e89a9 Compare October 29, 2024 14:25

Code review recommended minor version update not patch update.

5343372

shane-ns1 reviewed Oct 29, 2024

View reviewed changes

ns1/rest/zones.py Outdated Show resolved Hide resolved

ns1/rest/zones.py Outdated Show resolved Hide resolved

ns1/rest/zones.py Outdated Show resolved Hide resolved

Apply suggestions from code review

27a205a

Use f-strings and be more pythonic... Co-authored-by: Shane Kerr <133137948+shane-ns1@users.noreply.github.com>

cbertinato approved these changes Oct 29, 2024

View reviewed changes

shane-ns1 approved these changes Oct 29, 2024

View reviewed changes

hhellyer merged commit 0e0ac36 into master Oct 31, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zone file import - support missing params. #126

Zone file import - support missing params. #126

hhellyer commented Jun 21, 2024

hhellyer commented Jun 25, 2024

shane-ns1 left a comment

shane-ns1 Jun 25, 2024

shane-ns1 Oct 29, 2024

shane-ns1 Jun 25, 2024

shane-ns1 Jun 25, 2024

hhellyer Jun 28, 2024

shane-ns1 Jun 25, 2024

shane-ns1 Jun 25, 2024

cbertinato Jun 25, 2024

cbertinato Jun 25, 2024

hhellyer Jul 5, 2024

cbertinato Jun 25, 2024

cbertinato Oct 29, 2024

	:param str zone: zone name override, name will be zone FQDN if omitted
	:param str name: zone name override, name will be zone FQDN if omitted

Zone file import - support missing params. #126

Zone file import - support missing params. #126

Conversation

hhellyer commented Jun 21, 2024

hhellyer commented Jun 25, 2024

shane-ns1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment