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

Add remaining room APIs #1705

Merged
merged 5 commits into from
Apr 28, 2023
Merged

Add remaining room APIs #1705

merged 5 commits into from
Apr 28, 2023

Conversation

mjh1
Copy link
Contributor

@mjh1 mjh1 commented Apr 27, 2023

What does this pull request do? Explain your changes. (required)

Add in remaining room APIs for the multiparticipant feature

Specific updates (required)

How did you test each of these updates (required)

Does this pull request close any open issues?

Screenshots (optional)

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@mjh1 mjh1 requested a review from a team as a code owner April 27, 2023 11:05
@vercel
Copy link

vercel bot commented Apr 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2023 4:14pm

packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed
packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed
packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed
packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed
packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #1705 (a8a7923) into master (a7d2600) will increase coverage by 0.38428%.
The diff coverage is 85.45455%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1705         +/-   ##
===================================================
+ Coverage   52.78002%   53.16430%   +0.38428%     
===================================================
  Files             75          75                 
  Lines           4964        5009         +45     
  Branches         988         992          +4     
===================================================
+ Hits            2620        2663         +43     
- Misses          2007        2009          +2     
  Partials         337         337                 
Impacted Files Coverage Δ
packages/api/src/parse-cli.ts 34.04255% <ø> (ø)
packages/api/src/controllers/room.ts 58.25243% <85.45455%> (+28.94209%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49495a8...a8a7923. Read the comment docs.

Impacted Files Coverage Δ
packages/api/src/parse-cli.ts 34.04255% <ø> (ø)
packages/api/src/controllers/room.ts 58.25243% <85.45455%> (+28.94209%) ⬆️

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM with nits, the only a little stronger preference is for the egress API to receive a stream ID instead (or primarily) of a raw RTMP URL. Still not a required change tho.

packages/api/src/controllers/room.test.ts Outdated Show resolved Hide resolved
expect(res.status).toBe(400);

res = await client.post(`/room/${roomRes.id}/egress`, {
rtmpURL: "rtmp",
Copy link
Member

Choose a reason for hiding this comment

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

Could we allow at least sending a stream ID here, so it's not something alien to the rest of our API? (if you do so, make sure to check the stream belongs to the same user as the room / as the caller)

That should also be the recommended way for egress configuration, with an optional way of specifying a "third-party" stream. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right, it should definitely just accept a streamID, and the third party stream would be something to consider later on down the line, for now we've only discussed streaming to livepeer.

packages/api/src/controllers/room.test.ts Show resolved Hide resolved
packages/api/src/controllers/room.ts Show resolved Hide resolved
packages/api/src/controllers/room.ts Outdated Show resolved Hide resolved
"&token=" +
token,
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to edit the participants field in the room here? Or is it only updated on the webhooks?

Copy link
Contributor Author

@mjh1 mjh1 Apr 28, 2023

Choose a reason for hiding this comment

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

I was thinking we only really need to track who has actually joined the room (since we only start being charged at that point). But I guess it might be useful to keep track of when and what tokens have been generated, for debugging. WDYT?

packages/api/src/controllers/room.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/room.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/room.ts Outdated Show resolved Hide resolved
packages/api/src/schema/schema.yaml Outdated Show resolved Hide resolved
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed
@mjh1 mjh1 merged commit 10bcdb6 into master Apr 28, 2023
11 checks passed
@mjh1 mjh1 deleted the mh/room-apis branch April 28, 2023 17:10
suhailkakar pushed a commit that referenced this pull request May 4, 2023
* add more apis

* add tests

* add schemas

* record egress events

* address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants