Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-9174: add app wrappers #4561

Merged

Conversation

purplenicole730
Copy link
Member

@purplenicole730 purplenicole730 commented Nov 18, 2024

Adding all the app wrappers.

Notes:

  • Added a new common.go file to include constants and variables used by multiple clients.
  • Shadow types are in app_proto_conversions.go and registry_proto_conversions.go. Some shadow types are also used in ML Training, so these were kept secret to use as the ML Training client. Types that are used in both app and ML training will go into common.go, while everything else that remains will go into app_client.go and mltraining_client.go. When the ML training app wrappers are added, app_proto_conversions.go and registry_proto_conversions.go will be deleted.
  • Talking to @benjirewis and @jckras, we came to the conclusion that for TailRobotPartLogs, we can give the stream object itself to the user so that the user does what they want with it, whether they want to make it a channel or not. This is common practice in Go.

@viambot 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 22, 2024
@viambot 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 22, 2024
@viambot 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 22, 2024
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

One suggested type change for the Authorization struct and some questions around code safety, otherwise this lgtm.

app/app_client.go Outdated Show resolved Hide resolved
app/app_client.go Outdated Show resolved Hide resolved
app/app_client.go Outdated Show resolved Hide resolved
app/app_client.go Outdated Show resolved Hide resolved
app/app_client_test.go Outdated Show resolved Hide resolved
@viambot 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 25, 2024
@viambot 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 25, 2024
@viambot 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 25, 2024
@viambot 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 25, 2024
@viambot 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 25, 2024
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

lgtm!

@purplenicole730 purplenicole730 merged commit 56eab9a into viamrobotics:main Nov 25, 2024
16 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-9174-add-app-wrappers branch November 25, 2024 18:45
randhid pushed a commit to randhid/rdk that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants