RSDK-9194: streamline app connection #4516

purplenicole730 · 2024-11-01T19:54:46Z

Changes:

added new directory app/
introduced a ViamClient struct
made a function CreateViamClient, which takes in api key for credentials
add unit tests for url and api key formatting

Testing:

called a proto dataClient and tested with an existing machine
unit tests

jckras

Quick question - how are we going to implement the interfaces for other clients, such as DataClient and BillingClient, here? (In the Python sdk, we define data_client as a property of the ViamClient class). I'm not sure if this is outside the scope of this PR, but I just want to clarify our approach moving forward :)

jckras

I left a question about our approach towards building out the other clients w/ respect to ViamClient (but again maybe that is out of scope of this ticket) so LGTM.

app/viam_client_test.go

purplenicole730 · 2024-11-05T13:41:09Z

Quick question - how are we going to implement the interfaces for other clients, such as DataClient and BillingClient, here? (In the Python sdk, we define data_client as a property of the ViamClient class). I'm not sure if this is outside the scope of this PR, but I just want to clarify our approach moving forward :)

Similarly, the other clients would be part of ViamClient. But we don't have those yet so there's nothing to add. That's what the next tickets are.

njooma

I think it would be better if there was an Options struct that contained 2 fields: BaseUri string and Credentials rpc.Credentials. Then, have a func WithOptions that takes in these options.

What I think can be improved from the current version is that baseURL has to always be specified, even if just to be an empty string. Plus, there might be more ways in the future of connecting to app that this allows for.

And nothing is stopping us from making convenience functions like FromApiKey so that the user can use API Keys instead of Options

njooma · 2024-11-12T19:54:46Z

app/viam_client.go

+// APIKeyOptions has what is necessary to connect through GRPC with an API key.
+type APIKeyOptions struct {
+	baseURL  string
+	apiKey   string
+	apiKeyID string
+}


Delete this

app/viam_client.go

njooma · 2024-11-12T20:03:21Z

app/viam_client.go

+}
+
+// CreateViamClientWithAPIKey creates a ViamClient with an API key.
+func CreateViamClientWithAPIKey(ctx context.Context, apiKeyOptions APIKeyOptions, logger logging.Logger) (*ViamClient, error) {


This should take in normal Options and apiKey and apiKeyID

I am curious, if a user is going to pass in an Options object, at that point, wouldn't they just set the APIKey themselves?

app/viam_client_test.go

njooma · 2024-11-14T18:42:36Z

app/viam_client.go

+	if !validateAPIKeyFormat(apiKey) {
+		return nil, errors.New("API key should be a 32-char all-lowercase alphanumeric string")
+	}
+	if !validateAPIKeyIDFormat(apiKeyID) {
+		return nil, errors.New("API key ID should be an all-lowercase alphanumeric string with this format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx")
+	}


Ok to leave this in, but I do think it's unnecessary -- the server will tell the user that their creds are incorrect so no need to do client-side checking.

If it's unnecessary, then I'll take it out!

purplenicole730 added 2 commits November 1, 2024 14:07

create viam client function

bc2fa7f

user provides opts like in other sdks

1d55fe0

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 1, 2024

purplenicole730 added 3 commits November 1, 2024 17:12

change name of url

e40cdaf

add checks for api key and key id formatting

c5b41f5

add tests

effcf7f

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 4, 2024

make lint

7b80517

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 4, 2024

purplenicole730 added 2 commits November 4, 2024 15:13

mock dialdirectgrpc

78a16da

make lint

1b2b254

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 4, 2024

purplenicole730 marked this pull request as ready for review November 4, 2024 20:32

purplenicole730 requested review from a team, njooma and jckras and removed request for a team November 4, 2024 20:32

jckras reviewed Nov 4, 2024

View reviewed changes

jckras approved these changes Nov 4, 2024

View reviewed changes

app/viam_client_test.go Outdated Show resolved Hide resolved

njooma requested changes Nov 6, 2024

View reviewed changes

purplenicole730 added 2 commits November 7, 2024 12:28

create options struct

8e65f0a

add create function with apikey

3d12187

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 7, 2024

make lint

71ba42d

viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Nov 7, 2024

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 7, 2024

purplenicole730 requested a review from njooma November 7, 2024 22:24

njooma requested changes Nov 12, 2024

View reviewed changes

test empty url

660e03e

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 12, 2024

pr feedback

7e15fec

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 12, 2024

make lint

1b27971

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 13, 2024

purplenicole730 requested a review from njooma November 13, 2024 16:37

njooma approved these changes Nov 14, 2024

View reviewed changes

remove unnecessary client check

1d136d9

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 15, 2024

purplenicole730 merged commit 8f47d2c into viamrobotics:main Nov 15, 2024
16 checks passed

purplenicole730 deleted the RSDK-9194-streamline-app-connection branch November 15, 2024 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-9194: streamline app connection #4516

RSDK-9194: streamline app connection #4516

purplenicole730 commented Nov 1, 2024 •

edited

Loading

jckras left a comment

jckras left a comment

purplenicole730 commented Nov 5, 2024 •

edited

Loading

njooma left a comment

njooma Nov 12, 2024

njooma Nov 12, 2024

purplenicole730 Nov 12, 2024 •

edited

Loading

njooma Nov 14, 2024

purplenicole730 Nov 15, 2024

RSDK-9194: streamline app connection #4516

RSDK-9194: streamline app connection #4516

Conversation

purplenicole730 commented Nov 1, 2024 • edited Loading

jckras left a comment

Choose a reason for hiding this comment

jckras left a comment

Choose a reason for hiding this comment

purplenicole730 commented Nov 5, 2024 • edited Loading

njooma left a comment

Choose a reason for hiding this comment

njooma Nov 12, 2024

Choose a reason for hiding this comment

njooma Nov 12, 2024

Choose a reason for hiding this comment

purplenicole730 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

njooma Nov 14, 2024

Choose a reason for hiding this comment

purplenicole730 Nov 15, 2024

Choose a reason for hiding this comment

purplenicole730 commented Nov 1, 2024 •

edited

Loading

purplenicole730 commented Nov 5, 2024 •

edited

Loading

purplenicole730 Nov 12, 2024 •

edited

Loading