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.

Sign up for GitHub

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

Jump to bottom

Add SDK version, allow override of SDK and version from FFI #471

Merged
merged 24 commits into from
Oct 23, 2024
Merged

Add SDK version, allow override of SDK and version from FFI #471

merged 24 commits into from
Oct 23, 2024

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Oct 17, 2024

Two parts here:

  1. Send the rust SDK version for the RTC client
  2. Allow FFI-based clients to override SDK and version

@bcherry bcherry requested a review from theomonnom October 17, 2024 00:34
@bcherry bcherry mentioned this pull request Oct 17, 2024
Merged
bcherry
bcherry commented Oct 17, 2024
View reviewed changes
livekit-ffi/src/cabi.rs Outdated Show resolved Hide resolved
theomonnom
theomonnom reviewed Oct 17, 2024
View reviewed changes
livekit/src/room/mod.rs Outdated Show resolved Hide resolved
theomonnom
theomonnom reviewed Oct 18, 2024
View reviewed changes
livekit/src/room/mod.rs Outdated Show resolved Hide resolved
theomonnom
theomonnom reviewed Oct 18, 2024
View reviewed changes
livekit-ffi/src/cabi.rs Outdated Show resolved Hide resolved
theomonnom
theomonnom reviewed Oct 23, 2024
View reviewed changes
livekit-api/src/signal_client/mod.rs Outdated
Comment on lines 71 to 72
pub sdk: String,
pub sdk_version: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

let's put both of them to Option by default, just for consistency

Suggested change
pub sdk: String,
pub sdk_version: Option<String>,
pub sdk: Option<String>,
pub sdk_version: Option<String>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I don't really want to... there wasn't an existing case where the sdk wasn't set already

theomonnom
theomonnom reviewed Oct 23, 2024
View reviewed changes
livekit/src/room/mod.rs Outdated Show resolved Hide resolved
@bcherry bcherry merged commit 21149a4 into main Oct 23, 2024
10 of 16 checks passed
@bcherry bcherry deleted the bcherry/client-analytics branch October 23, 2024 23:08
@bcherry bcherry mentioned this pull request Oct 24, 2024
Merged
@mgsloan mgsloan mentioned this pull request Nov 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theomonnom theomonnom theomonnom approved these changes

@davidzhao davidzhao Awaiting requested review from davidzhao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bcherry @davidzhao @theomonnom