-
Notifications
You must be signed in to change notification settings - Fork 254
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
Implement REST API versioning and consolidate Job REST API endpoints #1521
Conversation
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.
No criticisms here. All in all pretty straightforward, however I just have some questions and some very minor documentation nitpicks.
I did not realize this would be implemented so quickly. I have some concerns about this approach, and I would be interested to understand the impact to the Nautobot ecosystem (SDK / ansible / nornir). |
We realized we needed a plan in place for 1.3 with the new Jobs REST API, so we had to make a decision and move quickly to avoid delaying the release or creating more tech debt.
Sure, fire away - what specific questions/concerns do you have? |
Co-authored-by: Jathan McCollum <jathan@gmail.com>
I am not really sure how the Nautobot ecosystem would be handled. Take for an example, if there are new APIs, how do we support the same level of backwards and forwards capabilities? Presumably there would need to be new API parameters, which would be difficult to document & accommodate within an Ansible module. Also, I don't think it would be clear that by default we would use the older version of the API. |
@@ -197,6 +211,12 @@ def initial(self, request, *args, **kwargs): | |||
""" | |||
super().initial(request, *args, **kwargs) | |||
|
|||
# Django Rest Framework stores the raw API version string e.g. "1.2" as request.version. | |||
# For convenience we split it out into integer major/minor versions as well. | |||
major, minor = request.version.split(".") |
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.
What is the result of this if 1.2.3
is sent in as the API version for example?
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.
That's not one of the versions listed in settings.REST_FRAMEWORK["ALLOWED_VERSIONS"]
so DRF would correctly reject it as an unknown/unsupported version. I don't think we want to get into the habit of doing patch release versioning of the API.
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.
Agreed on patch release API versioning. My curiosity is if it could/should safely/silently handle the patch version: 1.2.3
is treated as 1.2
is someone were to put the very specific installed version of Nautobot.
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.
From my understanding we'd either have to enumerate all of the patch versions for settings.REST_FRAMEWORK["ALLOWED_VERSIONS"]
or else do a custom subclass of AcceptHeaderVersioning
that discards the patch number from the version before checking against ALLOWED_VERSIONS
. Neither seems worth the trouble to me.
@itdependsnetworks Are any of your questions answered by the comments in the original issue? |
Fixes: #1465
NautobotAPIVersionMixin
.JobModelViewSet
andJobViewSet
classes and the separateapi/extras/jobs/
and/api/extras/job-models/
URL patterns into a combined viewset and URL patternapi/extras/jobs/
list URL - for API versions 1.2 and earlier (default) it still will return the job-class listing, whereas clients explicitly requesting API version 1.3 or later they will now get the new job-model listing./api/extras/jobs/<uuid:pk>/...
versus/api/extras/jobs/<str:class_path>/...
) and so do not actually need to enforce API versioning.