-
Notifications
You must be signed in to change notification settings - Fork 60
[controller] Add views to update general device information #164
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.
Looks good for now, looking forward to see the test, see below for a minor improvement
@@ -132,6 +135,9 @@ def get_controller_urls(views_module): | |||
url(r'^controller/download-config/(?P<pk>[^/]+)/$', | |||
views_module.device_download_config, | |||
name='download_config_legacy'), | |||
url(r'^controller/update-info/(?P<pk>[^/]+)/$', | |||
views_module.device_update_info, | |||
name='update_info_legacy'), |
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.
we can avoid the legacy URL since this is a new URL
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 am a little bit irritated because here it only works with the legacy URL ^controller/update-info/(?P<pk>[^/]+)/$
. And in openwisp-config
the URL is also $BASEURL/update-info/$UUID/
with $BASEURL
being $URL/controller
.
What am I missing?
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.
you're right, we haven't updated openwisp-config to use the new URLs yet, so please go ahead and put the legacy URL back since it seems we need it right now (the patch in openwisp-config is also using that one right?)
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.
Yes, openwisp-config
uses only URLs like ^controller/<api-endpoint>/
and not URLs like ^controller/device/<api-endpoint>/
.
See:
https://github.com/openwisp/openwisp-config/blob/master/openwisp-config/files/openwisp.agent#L73
https://github.com/openwisp/openwisp-config/blob/master/openwisp-config/files/openwisp.agent#L585
So we will need to keep the legacy URLs for quite some time, otherwise older clients won't be able to connect to an updated server anymore.
I re-added the legacy URL.
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.
PS: build is failing for an isort
warning.
c1470f2
to
f0012d6
Compare
f0012d6
to
fbead12
Compare
Fixed that and added tests. This is done from my side. |
fbead12
to
94c27d3
Compare
94c27d3
to
cdef54a
Compare
@nemesisdesign Please review this PR and the related ones for |
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.
@okraits on the code side It looks good, please give me some more time to test the changes in the 3 different modules (including openwisp-config) in my test env. This feature will go in the next release.
@nemesisdesign No problem, I have a week off from work now anyway. |
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.
@okraits first tests successful 👍
No description provided.