-
Notifications
You must be signed in to change notification settings - Fork 888
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 header() method to support HTTP/S Headers #890
base: master
Are you sure you want to change the base?
Add header() method to support HTTP/S Headers #890
Conversation
Fixes #882 |
The TravisCI check seems to be choking on something unrelated to this PR. I ran my tests locally and they worked fine. That TravisCI check is failing due to missing remote file @ https://www.johnvansickle.com/ffmpeg/builds/ffmpeg-git-64bit-static.tar.xz |
Yes, John changed his URLs, the new one is https://johnvansickle.com/ffmpeg/builds/ffmpeg-git-amd64-static.tar.xz |
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.
Overall I'm not a fan of this solution. There is the obvious problem that header options will be added to all inputs (which is incoherent with how fluent-ffmpeg normally handles input options). Also I feel like allowing multiple header calls to add multiple headers adds a LOT of complexity.
Given that using the headers option currently works with inputOptions
, maybe we could just add an alias helper that you can call as:
command.headers({header: value, header: value ...})
and whose implementation would simply be:
headers(headers) {
this.inputOption('-headers', Object.keys(headers).map(k => k + ': ' + headers[k]).join('\r\n'))
}
@@ -236,6 +237,7 @@ module.exports = function(proto) { | |||
|
|||
// For each input, add input options, then '-i <source>' | |||
return args.concat( | |||
headers, |
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.
This is incorrect. Headers options will be added to all inputs.
I'll take a look at implementing these suggestions. |
I'll make a separate PR to fix the test suite |
Tests are fixed, you can rebase on master |
Any updates on this? |
Headers are sometimes important for fetching source files from secure locations (i.e. Google Cloud Storage, AWS S3). This PR provides a clearcut method for adding HTTP headers with assurance that headers are applied in the right order on the
ffmpeg
andffprobe
commands.headers()
method added.Examples
ffmpeg
ffprobe