-
Notifications
You must be signed in to change notification settings - Fork 2
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
Notify studio when recording completes #50
Conversation
Triggers: Use switch to branch to proper pipeline logic based on stream name Move string constant from UploadVOD() to config
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 from studio side of things!
default: | ||
// Not related to API logic | ||
} | ||
} | ||
|
||
func (d *MistCallbackHandlersCollection) RecordingPushEnd(w http.ResponseWriter, req *http.Request, streamName, actualDestination, pushStatus string) { | ||
var err error | ||
pushSuccess := pushStatus == "null" |
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.
What is the expected behavior from Studio if this is false
? Should it show an error to the user somehow or can it still recover the recording somehow?
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.
Fail in our first version. Improve by recovering in later tasks. Same applies for transcoding stages where some of them can fail independently. To be optimal we can just re-run failed stages. For now we re-run entire process.
if len(path) < 4 { | ||
return "", fmt.Errorf("push url path malformed: element count %d %s", len(path), pushUrl.EscapedPath()) | ||
} | ||
return path[len(path)-2], nil |
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.
Should we have a regex here instead? Perhaps validating some other parts of this URL as well. (I particularly don't remember what it is, from this. is it the s3+https://
uri?)
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.
Good question. We now have plans to expand on URL path prefix with region and bucket name later. I figured its safer to grab path element before filename which should stay in this format in near future.
Codecov Report
@@ Coverage Diff @@
## main #50 +/- ##
===================================================
+ Coverage 33.17121% 34.88152% +1.71030%
===================================================
Files 17 17
Lines 1028 1055 +27
===================================================
+ Hits 341 368 +27
+ Misses 646 642 -4
- Partials 41 45 +4
Continue to review full report at Codecov.
|
No description provided.