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

Video thumbnail by key #21018

Merged
merged 3 commits into from Mar 6, 2018
Merged

Video thumbnail by key #21018

merged 3 commits into from Mar 6, 2018

Conversation

joshlory
Copy link
Contributor

@joshlory joshlory commented Mar 5, 2018

When we name videos by id, their id can change as we add new videos in the middle of videos.csv. If we happen to scoop content before thumbnails update, every thumbnail after the added video is off by one.

Instead name videos by key which doesn't change.

@joshlory joshlory requested a review from caleybrock March 5, 2018 22:48
Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

🙏 Logically this makes so much more sense, but I have one concern.

Although having a DTT fail with mismatching thumbnails is annoying, it also ensures we scoop thumbnails before deploying. My concern here is we might end up in a case where we are using a new video from LB, but the thumbnail gets left behind on staging. It'd only break the new video, but it wouldn't be great.

If it's gonna be a while before we add thumbnail generation to LB, I'm concerned about this. If the thumbnail generation is on its way, I think we can live with that risk for a bit.

@joshlory
Copy link
Contributor Author

joshlory commented Mar 6, 2018

Moving the cronjob code to happen on individual video save is next on my list 😄

@caleybrock
Copy link
Contributor

Perfect.

@joshlory joshlory merged commit 48014f8 into staging Mar 6, 2018
@joshlory joshlory deleted the video-thumbnail-by-youtube-code branch March 6, 2018 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants