Feature - asset download from s3 #61
Conversation
I haven't tested this yet because I need to switch over to my Windows partition where I have After Effects installed.
Calculating `src` with `url.parse` was causing problems with assets in my project containing spaces (since they get encoded as `%20`). With this change my POC of downloading assets from a private S3 bucket seems to be working
|
Hello @joshea0 Nice to see you again! Thank you for the pull request. I added a minor question to the code diff page. Thank you for the proper description and documentation, that will help a lot. Also, most likely i will merge this feature to the next version im currently working on. This is a preview of the same file, and nice spot for the s3 integration i prepared right there: https://github.com/inlife/nexrender/blob/version-1/packages/nexrender-render/src/tasks/download.js |
renderer/tasks/rename.js
Outdated
| @@ -19,7 +19,7 @@ module.exports = function(project) { | |||
|
|
|||
| // iterate over each file and create rename(move) callback | |||
| for (let asset of project.assets) { | |||
| let src = path.join( project.workpath, path.basename(url.parse(asset.src).pathname)); | |||
| let src = path.join( project.workpath, asset.src.substring( asset.src.lastIndexOf('/') + 1 )); | |||
There was a problem hiding this comment.
could you please explain the reason for this change? maybe some example of why its needed
There was a problem hiding this comment.
I'm glad you asked, because while writing up an explanation I realized my original reasoning for this change was incorrect. I made this change to solve an issue I was having with spaces in file names, but now that I think through it again I don't think this was the best way to fix it.
If you look at my first commit, I was calling downloadFromS3(asset.bucket, asset.key, project.workpath, asset.name) - this meant if I had a file like my video.mp4 it would get downloaded and saved as my video.mp4, but then path.basename(url.parse(asset.src).pathname) in rename.js would look for my%20video.mp4 (because url.parse encodes spaces as %20) and the project would fail at that stage.
I changed both pieces to use asset.src.substring( asset.src.lastIndexOf('/') + 1 ) to grab the file name out of the URL without using url.parse and that fixed my issue. However, now that I think about it I probably should have just changed my call to downloadFromS3 to use url.parse as well, like so: downloadFromS3(asset.bucket, asset.key, project.workpath, path.basename(url.parse(asset.src).pathname)) then there would be no change required to rename.js
Does that make sense? I'm going to push another commit with this change so you can see what I mean. Changing this should fix the failing tests as well (my mistake for not running the tests before submitting the PR).
This should fix the failing tests from my previous commit. While trying to fix an issue with spaces in file names being encoded as `%20` I made a change to rename.js and download.js (see 6af43ff) which was a bit short-sighted. After reviewing it, I think reverting the change to rename.js and changing the S3 portion of download.js to use `path.basename(url.parse(asset.src).pathname)` to match the behavior in `rename.js` is a better fix to the issue. See here for context / explanation: inlife#61 (comment)
renderer/tasks/download.js
Outdated
| @@ -52,8 +53,7 @@ module.exports = function(project) { | |||
| // iterate over each asset and download it (copy it) | |||
| Promise.all(project.assets.map((asset) => { | |||
| if (asset.type === 's3') { | |||
| let dstFile = asset.src.substring( asset.src.lastIndexOf('/') + 1 ); | |||
| return downloadFromS3(asset.bucket, asset.key, project.workpath, dstFile); | |||
| return downloadFromS3(asset.bucket, asset.key, project.workpath, path.basename(url.parse(asset.src).pathname)); | |||
There was a problem hiding this comment.
@inlife since path.basename(url.parse(asset.src).pathname) is now used in both download.js and rename.js, do you think it should be refactored into a function somewhere?
I'm not sure where would be the best place to put it, but since we want the dstName parameter passed to downloadFromS3 to always match the 2nd param on this line in rename.js:
let src = path.join( project.workpath, path.basename(url.parse(asset.src).pathname));
maybe it makes sense to have them share some helper function?
|
@inlife thanks! Glad I finally found the time to get back in here. Thanks again for all your work on this project, it has been immensely helpful in the past few months. I replied to your question and pushed another commit which fixes the 3 tests that were failing. The preview of the next version looks great! I am excited for the 1.0 release - it looks like you've put a lot of work into it. I definitely see a few features in there I am looking forward to using (e.g., secret based security, https, license creator task). Let me know if there are any areas I can help you out with the release. |
|
Would it be better for me to refactor this so it can be merged into the |
@joshea0 thank you so much for that! )
i will probably just move them both to setup task, where they will be assigned to asset.name (in case it isnt set up)
I merged it in master for now, considering the fact it works and it's tested, i dont see why not. And the new version might take an unexpected amount of time to finish.
Actually there is one thing: when i was developing the first 0.1 version of nexrender, i wanted to have post-render tasks, actions. The ones that will upload the video to youtube or email it, or do whatever. However after some time, it seemed to me that this is probably should not be this project responsibility, especially considering how many different possible scenarios for upload are possible (we just cant have them all). But the thing is, there is no easy way to handle your own post render tasks after render, in case you are doing it via render agent (formerly rendernode) and this is bad. And i dont know how to solve it. Having post rendering tasks would probably help in this case... What do you think? |
That makes a lot of sense. The other change I made in my fork besides the S3 download was to support a post rendering task of uploading videos.
Is your vision for a post-render task to allow users to specify arbitrary pieces of code that run after render? Or are you thinking a set of predefined tasks that can be controlled via configuration (e.g., "email video to this address", "upload the video to this FTP server")? I'm leaning towards the 2nd one, but I wanted to clarify what your vision is for this.
I agree that we can't cover every possible scenario. However, I'm thinking it may be possible to provide support for a few use cases which cover the majority of scenarios. Do you have some examples you could provide of what types of post-render tasks users have requested? I have some in mind, but I'd like to try to cover everyone's use cases and not just mine. I have started writing up some ideas for how I think this feature could work. I am going to open a new issue for it so we can discuss it there. |
oke, sounds good |
This should fix the failing tests from my previous commit. While trying to fix an issue with spaces in file names being encoded as `%20` I made a change to rename.js and download.js (see 6af43ff3d9b2321fff415b9bfa54086b125aa60f) which was a bit short-sighted. After reviewing it, I think reverting the change to rename.js and changing the S3 portion of download.js to use `path.basename(url.parse(asset.src).pathname)` to match the behavior in `rename.js` is a better fix to the issue. See here for context / explanation: inlife/nexrender#61 (comment)
Feature overview
This feature allows you to download assets from private S3 buckets using AWS credentials for authorization. It adds a new asset type "s3"
Setting up AWS credentials
For this to work, you will have to set up the proper AWS credentials on the renderer (the API server does not need the credentials). You can find info on setting that up here.
The credentials set will need to have read access for any of the S3 assets you wish to use.
Example S3 asset
Here is an example of the data structure needed for adding an S3 asset to your project:
{ "type": "s3", "name": "your_file_name.mp3", "bucket": "your-s3-bucket-name", "key": "the/s3/key/to/your/file.mp3", "src": "s3://your-s3-bucket-name/the/s3/key/to/your/file.mp3" }I do not have a test to provide with this feature at this time, but I have been using it in a couple of projects for a few months now. I'm not sure how to best go about testing it, so if you have any ideas on that I would greatly appreciate them!
Please let me know if there's anything I can add or improve in regards to the documentation or code for this feature. I am really grateful for this project and I'd like to do everything I can to make it even better.