clients: add API client to this SDK #197
Conversation
faculty/clients/api.py
Outdated
| if default_server_size is None: | ||
| default_server_size = { | ||
| "instanceSizeType": "custom", | ||
| "instanceSize": {"milliCpus": 1000, "memoryMb": 4096}, | ||
| } | ||
| payload["defaultServerSize"] = default_server_size |
There was a problem hiding this comment.
There might be some common schemas with the ServerClient might be worth considering reusing?
There was a problem hiding this comment.
Yeah, there are some stuff here and there, there's _InstanceSizeSchema from JobClient for example, but will see to mix and match further. It would be great to avoid duplication, there are some subtle differences as much as I can tell, but still digging into the actual platform api schema.
faculty/clients/api.py
Outdated
| ERROR = "error" | ||
|
|
||
|
|
||
| @define |
There was a problem hiding this comment.
Rather than attrs new interface, I'd actually advocate for using dataclasses now that we're only supporting 3.6+ (dataclasses are 3.7+, but there is a 3.6 backport that you can install on Python 3.6 only). The syntax is basically the same but this has the obvious advantage of being in the standard library and better integrated with other Python tools.
I'd suggest we use dataclasses here and gradually migrate other clients to use dataclasses in future. What do you think?
There was a problem hiding this comment.
Yeah, I think this is a good call. Don't have strong reason to go with attr, not given how do we use things.
The plan sounds very sensible, will do the changes here, and once merged, will create an issue to track the whole migration as you outline.
faculty/clients/api.py
Outdated
| working_directory: str | ||
| module: str | ||
| wsgi_object: str | ||
| last_updated_at: Optional[datetime] = None |
There was a problem hiding this comment.
From looking at the aperture code, I don't think this is Optional (see ControllerReads line 92 and PersistentDetails line 43).
There was a problem hiding this comment.
It is optional, because this class is used not just for replies from aperture, but also request for creating APIs, and then the user would need to supply a last_updated_at value.
ie. with the current Optional setting, we can do
api_definition = WSGIDefinition(working_directory="/project/testapi", module="app", wsgi_object="app")
# [...snip...]
x = client.create(project_id=PROJECT_ID, command_definition=api_definition, subdomain=subdomain, name=name, default_server_size=server)and without it the request will fail, obviously, unless a value is set, even if the create endpoint doesn't require that value (see the Command Definitions in the README).
Thus the tricky place is that for requests it's definitely optional, for replies it will always be included, and thus the reason why I did it like this.
Given all the above, how would you prefer to resolve it?
- keeping it
Optionaldue to the dual use? - making it non-optional but having a default value? (of
datetime.now()or something?) (this feels dirty) - making it non-optional and make people pass in some value? (feels dirty like above)
- something completely different?
There was a problem hiding this comment.
Hmm, the "right" thing to do here would be to have different models, since the schema for the create body is different than the schema for the get response. But that will cause a fair bit of duplication, which is obviously undesirable. I'll have a think and get back to you.
I'd say a definite no to the second and third bullets in your comment.
There was a problem hiding this comment.
Hey @acroz, I've pushed some changes to see if they resolve this conundrum, breaking classes up into "Something" (for requests) and "SomethingResponse" (with inheritance derived from Something, for responses). This feels like having the right amount of duplication (ie. "SomethingResponse" only needs the additional or changed fields). I've tested it very briefly so far and seems to work, also the way we are creating the json payload, that's helpful that we don't need to convert Something <-> SomethingResponse at any point, and still can use them. (that conversion would be a bit verbose and hacky, as much as I've researched).
It's not fully fledged yet (ie. there might be mistakes in the rewrite), but wanted to get your opinion with code.
faculty/clients/api.py
Outdated
| class PlumberDefinition: | ||
| working_directory: str | ||
| script_name: str | ||
| last_updated_at: Optional[datetime] = None |
faculty/clients/api.py
Outdated
| class ScriptDefinition: | ||
| working_directory: str | ||
| script_name: str | ||
| last_updated_at: Optional[datetime] = None |
The advantage being standard Python (for Python 3.7+).
This PR is a WIP, opening it for early feedback, will update description when closer to completion.