-
Notifications
You must be signed in to change notification settings - Fork 25
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
Client WebApp API #44
base: master
Are you sure you want to change the base?
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.
Refactorize the WebApp classes for simplicity (see DSSWiki & DSSWikiSettings as 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.
small reqs
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.
should not store the state within the webapp, otherwise, cannot refresh the state, even by calling get_state on the webapp object
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.
better future handling
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.
all good, if you want you can add a new function in DSSWebAppBackendState get_future that returns a DSSFuture with the jobId stored in DSSWebAppBackendState so the loading state of the webapp backend can be handle more easily
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.
LGTM
@@ -0,0 +1,82 @@ | |||
from time import sleep |
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 would remove this test class as it cannot be run automatically
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.
Talk with @Basharsh96 first :)
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 made a copy of the test and I'll add it later to the tests in the CI pipeline I'm working on. Thank you, you can safely remove it 😊
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.
API modernization
# Webapps | ||
######################################################## | ||
|
||
def list_webapps(self): |
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.
Adopt the newer standard of returning either listitem or object. See list_scenarios for an up-to-date example. This allows basic listing with a single API call instead of N+1
Provide properties on the core things in the DSSWebAppListItem
A handle to manage a webapp | ||
""" | ||
def __init__(self, client, project_key, webapp_id): | ||
"""Do not call directly, use :meth:`dataikuapi.dss.project.DSSProject.get_webapps`""" |
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.
typo
""" | ||
Stop a webapp | ||
""" | ||
self.client._perform_empty("PUT", "/projects/%s/webapps/%s/backend/actions/stop" % (self.project_key, self.webapp_id)) |
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.
Check with @FChataigner whether it's a good thing that this does not return a future
:rtype: :class:`dataikuapi.dss.future.DSSFuture` | ||
""" | ||
future = self.client._perform_json("PUT", "/projects/%s/webapps/%s/backend/actions/restart" % (self.project_key, self.webapp_id)) | ||
return DSSFuture(self.client, future["jobId"]) |
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.
use DSSFuture.from_resp which properly handles the case where the future instantly succeeded or failed (here, it will do KeyError on jobId
""" | ||
self.client._perform_empty("PUT", "/projects/%s/webapps/%s/backend/actions/stop" % (self.project_key, self.webapp_id)) | ||
|
||
def restart_backend(self): |
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.
While this corresponds to what the API really does, the fact that there is a "restart" method but no "start" method could be confusing.
Not sure what the best to do is here. Maybe call the method start_or_restart_backend
self.webapp_id = webapp_id | ||
self.state = state | ||
|
||
def get_state(self): |
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.
Make this a property
""" | ||
return self.state | ||
|
||
def is_running(self): |
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.
Make this a property called "running"
self.webapp_id = webapp_id | ||
self.definition = definition | ||
|
||
def get_definition(self): |
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.
So this would be webapp.get_definition().get_definition() :) Standard practice is to call this method get_raw
so it will be webapp.get_settings().get_raw()
""" | ||
return self.definition | ||
|
||
def set_definition(self, definition): |
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, no set function on a "modern" settings object. It's modification in place.
@@ -0,0 +1,82 @@ | |||
from time import sleep |
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.
Talk with @Basharsh96 first :)
Implementation of a WebApp API client + TU