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

schema: add user.new event #995

Merged
merged 2 commits into from
Apr 29, 2022
Merged

schema: add user.new event #995

merged 2 commits into from
Apr 29, 2022

Conversation

gioelecerati
Copy link
Member

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

Added mist user.new in the event schema and the frontend subscription dropdown.

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.

@gioelecerati gioelecerati requested a review from a team as a code owner April 7, 2022 17:58
@vercel
Copy link

vercel bot commented Apr 7, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/livepeer/livepeer-com/wYypp7F4By7BycvE3cYrVjgXePKF
✅ Preview: https://livepeer-com-git-gio-apiaddusernewevent-livepeer.vercel.app

@@ -209,6 +209,7 @@ components:
- multistream.connected
- multistream.error
- multistream.disconnected
- user.new
Copy link
Member

Choose a reason for hiding this comment

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

@iameli I had talked to Gioele about this in a call and he mentioned that you were against naming this event differently in our API. I think I understand where you are coming from, not to have too much divergence between our API and Mist's, but I think on this case it is not a good idea to use the same terminology here. Simply because the "user" concept already exists and has a totally different meaning in the API, even with it's own /api/user root and all...

So when we do work on the Livepeer+Mist common API we can think of a better name for this, which makes sense in both contexts, but IMO right now using the name from one in another will only make things more confusing. Especially because currently the LP API and the Mist API have their own universe/namespaces so they will both need to be rethinked.

So @iameli WDYT? Could we rename this to something that makes more sense in the livepeer.com/api? If we were going totally free I'd go maybe with:

  • stream.playback.new, but if we want to keep some level of consistency with Mist, WDYT of;
  • stream.user.new or maybe even
  • stream.playback.user.new? (I actually like this last one)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense. Do you think there's anything in the fact that this same hook would likely be (eventually) used to authorize VoD playbacks as well, perhaps making the stream prefix less meaningful? What about just playback.new?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Could also be playback.user.new

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to playback.user.new both here and here!

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!

@iameli
Copy link
Member

iameli commented Apr 11, 2022

@gioelecerati Looks great. Let's sit on this for a bit until we're ready with the backend for it 👍

@iameli
Copy link
Member

iameli commented Apr 29, 2022

Mergin'.

@iameli iameli merged commit 2590a39 into master Apr 29, 2022
@iameli iameli deleted the gio/api/add_user_new_event branch April 29, 2022 18:27
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

4 participants