-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(api): Push target API #430
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/livepeerorg/livepeer-com/9MoyBhueaFErVAoSNgN8oR2eFqjH |
f842a44
to
4fbd895
Compare
d8a1dcd
to
a4aa7d3
Compare
bde63ae
to
b1402e0
Compare
c1a245f
to
e481360
Compare
34370eb
to
948b07e
Compare
Also a new test for cross-user push targets validation
I was wanting something like that already. Used the one from stream test as inspiration.
068077b
to
b037d23
Compare
FTR I believe this is ready for review now :D |
Right on, taking a look now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Mostly relatively minor changes; I'm going to boot this up locally and then give it one final go-through before we ship.
Co-authored-by: Eli Mallon <eli@livepeer.org>
Co-authored-by: Eli Mallon <eli@livepeer.org>
@iameli Ready for another round! Addressed most comments and replied to some others, just waiting on your position on them to make any related updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚢
Merge it and see if the API works on Livepeer.monster 😃
* fix client server mismatch * update language of the docs * add parent stream to session api response (#446) * feat(api): Push target API (#430) * Add some @types packages for push-target API - @types/express - @types/uuid Required fixing some function in www since it know started being typechecked because @types/qs showed up in the repository. * Copy object-store controllers into push-target * Make the push-target files TypeScript * Some additional typings to db & table * Create push target schema and DB Mostly copied from object-store stuff. * Add some cool validators to url field * Create fields in stream object for push target * TODO push-target/profile validaiton * Allow for inline push-target in create stream * KISS inline push-target * Several updates to the schema - Rename "inline" to "spec" - Improve some descriptions and examples - Create missing GET/PATCH/DELETE push-target endpoints - Add readOnly/writeOnly to some fields so they only show in specific APIs * Implement push-target list API * Clean response objects if not an admin * Implement get API and a clean response middleware * Implement the remaining APIs Some schema updates were necessary * Create some error helpers * Some final nitpicks * Push Target ref/config validation on stream * pretty all the things * Create separate PushTargetTable helper logic Will avoid repeating some of the logic in both the stream and push target controllers. Included for example a default value for the target name, being consisted of the push URL host. Also included some other minor changes like TODOs and fixes to schema/formatting of some files. * Create unified stream patch API with push-target I found 2 different PATCH APIs in the /stream endpoint, each of them changing separate fields. I would be creating a 3rd one for just another field, but decided it was interesting to have a single patch instead which can be used for changing any of the mutable fields depending on the payload. So I did that. And also ended up making our schemas support references to one another since the pain was too big to implement all the schema validation by hand or to repeat once more all those push-target fields. * Implement the first test for push-target This involves fixing a few more general issues on the implementation. Next steps should be pretty much localized now. * Some more tests on listing API * Implement test on push-target pagination * Implement push-target creation tests * Last test under JWT section * Make having the non admin token the default Tests seem safer that way. * Tests for API key This finishes the push-target tests * Allow running tests on really silent mode By pasing a --silent flag, now all the loggers will shut themselves, and we'll get only the jest tests output for checking test run results. Bonus is running `yarn test --silent --watch` which now works pretty well. * Create a test for the default push target name * Tests for push-target on stream creation * Create a stream patch tests Also a new test for cross-user push targets validation * Create helper setup functions on push-target tests I was wanting something like that already. Used the one from stream test as inspiration. * nit on getNextCursor helper * Some tests for push-target patch * a couple final tests/nits * Update packages/api/src/schema/schema.yaml Co-authored-by: Eli Mallon <eli@livepeer.org> * Update packages/api/src/schema/schema.yaml Co-authored-by: Eli Mallon <eli@livepeer.org> * Make abbrevs all-caps (WithID+DBPushTarget) * Rename other 'aac' profile name examples to '720p' Co-authored-by: Eli Mallon <eli@livepeer.org> * fix(api): Fix some small issues in the Push Target API (#449) * Avoid making pushTargets field null That doesn't even pass the JSON schema validation and might be error-prone for developers to consume. * Fix field cleaning on list API * add parent stream (#459) * expose headers to browser (#464) * update docs, add a paywall section and fix up language / layout * docs formatting update * docs language update * add video tutorial blog link to docs Co-authored-by: Julian Benegas <julianbenegas99@gmail.com> Co-authored-by: Adam Soffer <adam@livepeer.org> Co-authored-by: Victor Elias <victorgelias@gmail.com> Co-authored-by: Eli Mallon <eli@livepeer.org>
What does this pull request do? Explain your changes. (required)
This creates a new API object called a (RTMP(S) or SRT) Push Target, which is
meant for registering additional destinations that should receive a stream contents.
This has multiple meanings in the market like multi-cast, simulcast, re-streaming, etc.
Specific updates (required)
fully on TS
/push-target
root path segmentpushTargets
field inside thestream
object to reference existingpush targets or create a new one inlined in the stream creation payload.
How did you test each of these updates (required)
yarn test --silent
for now, which I believe are pretty comprehensiveDoes this pull request close any open issues?
Implements #431
Checklist: