RSDK-4528 - Allow API keys for cloud apis + allow users to specify app url #424
Conversation
cheukt
requested review from
clintpurser
and removed request for
clintpurser
cheukt
changed the title
src/viam/app/viam_client.py
Outdated
| if dial_options.credentials.type == "robot-location-secret": | ||
| self._location_id = dial_options.auth_entity.split(".")[1] | ||
| if app_url is None: | ||
| app_url = "https://app.viam.com" |
There was a problem hiding this comment.
(q) previously we were using "app.viam.com:443" as a hardcoded value, rather than "https://app.viam.com". is this change meaningful?
There was a problem hiding this comment.
not particularly, but figured specifying https:// is slightly better than specifying the port number
There was a problem hiding this comment.
tho neither is necessary, I could make it just app.viam.com if that's better
src/viam/rpc/dial.py
Outdated
| @@ -30,7 +30,7 @@ class Credentials: | |||
| Currently only supports robot location secret. | |||
There was a problem hiding this comment.
[minor] looks like we can remove this line. Yay!
| @@ -30,7 +30,7 @@ class Credentials: | |||
| Currently only supports robot location secret. | |||
| """ | |||
|
|
|||
| type: Union[Literal["robot-location-secret"], Literal["robot-secret"]] | |||
| type: Union[Literal["robot-location-secret"], Literal["robot-secret"], Literal["api-key"]] | |||
There was a problem hiding this comment.
[q] Should we add validation on this field such that users would get an error if they supplied a bad literal/typo? I'm surprised I was able to supply the api-key string in the original org API key PR without hitting any errors.
There was a problem hiding this comment.
it's just a type hint right now, so no error - the idea was that users could supply other cred types as the server changes without changes in the SDK, which I think is nicer - the server can/will reject if the type is incorrect anyway. I do think we could perhaps prepopulate these types, but out of scope for now (https://viam.atlassian.net/browse/RSDK-4756 could/should handle this better)
| async def _dial_app(app_url: str) -> Channel: | ||
| return await _dial_direct(app_url) |
There was a problem hiding this comment.
[q] I see that above in this file we do:
host, port = _host_port_from_url(address)
if not port:
port = 80 if insecure else 443
server_hostname = host
Should we maybe have it default to 8080? Were you able to test this both with localhost:8080 as well as http://localhost? I am not sure if it would work for the latter option as-is but haven't tested. We could potentially match on the string localhost (rather than requiring the http:// prefix) and hardcode 8080 in that case.
There was a problem hiding this comment.
localhost:8080 definitely works, so does http://localhost if I update the default to be 8080. I think we kept it as 80 because that's default insecure port
There was a problem hiding this comment.
ok! I'm not sure I follow why keeping it as 80 as desirable but I'm sure I'm lacking a lot of context here. Going to hit approve. Thank you!
RSDK-4528
location_id can apparently be none in app_client, so no changes needed there
allowing users to specify app url will make it easier for testing and stuff