change aws s3 cli to aws-sdk

[yoution]

No description provided.

[maxceem]

In general, works good.

Though there is one performance issue. Before, we use AWS CLI move command which moves the file without downloading it. Now the script first downloads the file and after uploads, for the big files it would be very slow, and most likely request for creating an attachment would fail after reaching the timeout.

1. To fix it, we can replace get and put operations with one copy operation, so we don't have to download the file and it should work much faster as it would copy the object between the buckets internally by AWS https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#copyObject-property.

2. Let's catch error during delete operation separately, so it doesn't break the whole s3FileTransfer method. So if we copy file successfully, but couldn't delete it, s3FileTransfer still return success. We still have to log an error if deleting failed for debugging.

[yoution]

@maxceem please review again, I use setTimeout to do the delete action, please comment
the code https://github.com/topcoder-platform/tc-project-service/blob/develop/src/routes/attachments/create.js#L73-L78 to validate

[maxceem]

It works great now.

I did a few small improvements, just FYI, don't have to fix it:

We should use req.log.error instead of req.log.debug for errors so in the log we can filter records and find all the errors:

[yoution]

thanks, your method is better then mine, promise is faster then settimeout

[vikasrohit]

Thanks @yoution and @maxceem for the efforts.

[vikasrohit]

@maxceem can we create hotfix instead of feature fix?

[maxceem]

@vikasrohit I was thinking that we would make a release this week anyway, so created a feature branch. Also, it's better to test it on DEV a little.

Another thing, it looks like we have a redundant code there and unnecessarily call uploadurl File Service endpoint: https://github.com/topcoder-platform/tc-project-service/blob/feature/file-transfer-s3-improvements/src/routes/attachments/create.js#L73-L90 - I think we can just remove this call.

So after testing this fix, I was going to remove that call and also test without it.

Let me know if you still would like to have a hotfix for this. Should hotfix include the second part with removing unnecessary call or no?

[vikasrohit]

Makes sense. I was only worried if by any unavoidable reason, we have to push the release. Most probably we should be releasing on Thursday. However, I think we do need some time in dev to test out the changes, so lets keep it feature for now and in case we don't have release this week on Thursday, we would make it hotfix.

[maxceem]

@vikasrohit it looks like on DEV file uploading is not fully enabled. Files are visually uploaded but I cannot open them after. Could we enable file uploading on DEV for now, so we can fully validate it?

[vikasrohit]

I think last time we made it enabled. I am not sure why it is not working now. I can check again, it was an env variable which used to prevent the actual file upload in dev.