RSDK-4529 - add missing app apis #478
Conversation
Co-authored-by: Maxim Pertsov <[email protected]>
Co-authored-by: Maxim Pertsov <[email protected]>
Co-authored-by: Maxim Pertsov <[email protected]>
Co-authored-by: Maxim Pertsov <[email protected]>
src/viam/app/app_client.py
Outdated
| Use this class when constructing API key authorizations to minimize the risk of malformed or missing data. | ||
| """ | ||
|
|
||
| def __init__(self, role: str, resource_type: str, resource_id: str): |
There was a problem hiding this comment.
You might want to make these type hints Union[Literal["owner"], Literal["operator"]] and Union[Literal["organization"], Literal["location"], Literal["robot"]]
| _role: str | ||
| _resource_type: str | ||
| _resource_id: str |
There was a problem hiding this comment.
Is there a reason for these being private?
There was a problem hiding this comment.
In general I favor minimizing direct access to data members. In this case the class is kind of weird/bespoke compromise (it's an opinionated version of the API key authorization information for a single, specific use case) and so I favored minimizing the amount of interaction users have with it. It's not a super strong preference though, happy to revisit if you disagree!
There was a problem hiding this comment.
Nope I like your reasoning, we can keep this
| identity_type="", | ||
| role=role, |
There was a problem hiding this comment.
Same note as above, perhaps make the parameter type hints be unions of literals
There was a problem hiding this comment.
Outside the scope of this PR, we might want to discuss further in depth the practice of having optional organization_id in these methods. For example, the delete_organization defaults to deleting the current organization. But then we never reset the _organization_id on the app client, meaning that any requests that come after will error, because the organization has been deleted. Plus, there's no way for the user to set the current org. This really feels like app state management, which I don't think the SDK should be a part of. Anyway, far outside the scope of this PR.
No description provided.