Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/viam/app/viam_client.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Mapping
from typing import Mapping, Optional

from grpclib.client import Channel
from typing_extensions import Self
Expand All @@ -20,13 +20,14 @@ class ViamClient:
"""

@classmethod
async def create_from_dial_options(cls, dial_options: DialOptions) -> Self:
"""Create `ViamClient` that establishes a connection to app.viam.com.
async def create_from_dial_options(cls, dial_options: DialOptions, app_url: Optional[str] = None) -> Self:
"""Create `ViamClient` that establishes a connection to the Viam app.

Args:

dial_options (viam.rpc.dial.DialOptions): Required information for authorization and connection to app. `creds` and
`auth_entity` fields are required.
app_url: (Optional[str]): URL of app. Uses app.viam.com if not specified.

Raises:
ValueError: If the input parameters are missing a required field or simply invalid.
Expand All @@ -42,16 +43,20 @@ async def create_from_dial_options(cls, dial_options: DialOptions) -> Self:
raise ValueError("dial_options.auth_entity cannot be None.")

self = cls()
self._location_id = dial_options.auth_entity.split(".")[1]
self._channel = await _dial_app(dial_options)
self._location_id = None
if dial_options.credentials.type == "robot-location-secret":
self._location_id = dial_options.auth_entity.split(".")[1]
if app_url is None:
app_url = "app.viam.com"
self._channel = await _dial_app(app_url)
access_token = await _get_access_token(self._channel, dial_options.auth_entity, dial_options)
self._metadata = {"authorization": f"Bearer {access_token}"}
return self

_channel: Channel
_metadata: Mapping[str, str]
_closed: bool = False
_location_id: str
_location_id: Optional[str]

@property
def data_client(self) -> DataClient:
Expand Down
11 changes: 4 additions & 7 deletions src/viam/rpc/dial.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,9 @@

@dataclass
class Credentials:
"""Credentials to connect to the robot.
"""Credentials to connect to the robot and the Viam app."""

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"]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Member Author

@cheukt cheukt Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

"""The type of credential
"""

Expand Down Expand Up @@ -314,5 +311,5 @@ async def dial_direct(address: str, options: Optional[DialOptions] = None) -> Ch
return await _dial_direct(address, options)


async def _dial_app(options: DialOptions) -> Channel:
return await _dial_direct("app.viam.com:443")
async def _dial_app(app_url: str) -> Channel:
return await _dial_direct(app_url)
Comment on lines +314 to +315
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!