grunt-s3
grunt-s3 copied to clipboard
pifantastic
migrating from Knox to Amazon's SDK
Uploads fail with InvalidParameterType: Expected params.Expires to be a Date object, ISO-8601 string, or a UNIX timestamp
I have tried:
'Expires': 1438992000
'Expires': new Date(Date.now() + 315360000000)
'Expires': new Date(Date.now() + 315360000000).toISOString()
And got the same result.
@c2h5oh I don't think those formats are right. It expects a UTC string. Example:
'Expires' : new Date(Date.now() + 315360000000).toUTCString()
Based on the error message I've quoted above all three formats should be acceptable.
Also I've only tried them after 'Expires' : new Date(Date.now() + 315360000000).toUTCString() I've been using with knox plugin version stopped working after switching to your branch.
Hmm, I don't see any of the supported formats in the knox or grunt-s3 documentation. I'm wondering if the knox Expires header is more accepting of various input formats. The aws-sdk that backs my branch is more strict.
@pifantastic should we bump the major version number to indicate API breaking changes, or would you prefer to try to shim all of the differences between knox and aws-sdk? I'm happy to help with this but I haven't heard any feedback from you, and I've put a lot of time into this patch already. I'd like to get your thoughts on this before I dump a bunch more time into it.
+1 for version bump
Drew Wilson http://geedew.com
On Tue, Aug 13, 2013 at 11:05 AM, Mike Reinstein [email protected]:
Hmm, I don't see any of the supported formats in the knox or grunt-s3 documentation. I'm wondering if the knox Expires header is more accepting various of input formats. The aws-sdk that backs my branch is more strict. @pifantastic https://github.com/pifantastic should we bump the major version number to indicate API breaking changes, or would you prefer to try to shim all of the differences between knox and aws-sdk. I'm happy to help with this but I haven't heard any feedback from you, and I've put a lot of time into this patch already. I'd like to get your thoughts on this before I dump a bunch more time into it.
— Reply to this email directly or view it on GitHubhttps://github.com/pifantastic/grunt-s3/pull/85#issuecomment-22576125 .
How difficult would it be to make expires compatible with knox? If that's the only breaking change, I'd rather not do a minor version bump.
I'd also like to point out that this is great work @mreinstein and I really appreciate it.
I think this PR is needed for #72 via (http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/frames.html) needing to know what's actually in the bucket to remove it. Something Knox does not have the ability to do AFAIK
[edit] Actually it is possible
client.list({ prefix: 'my-prefix' }, function(err, data){
http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketGET.html
Sorry for the delay in reply, crazy week!
How difficult would it be to make expires compatible with knox?
Probably not hard.
If that's the only breaking change, I'd rather not do a minor version bump.
The thing that scares me the most is that grunt-s3 is to a large extent a pass-through to the underlying file upload library. I suspect a lot of the allowed inputs that knox uses are different from aws-sdk. We could shim these, and I'm worried about the possibility of certain calls supporting features that mysteriously go away or change when we move to aws-sdk. It might be safer to declare API breakage rather than put a lot of work into shimming, and still have breaking changes leak through inadvertently. That's just a gut feeling though, open for discussion. Thoughts?
I'd also like to point out that this is great work @mreinstein and I really appreciate it.
My pleasure! Great library, happy to help.
Hey guys @pifantastic is trying to take a break here, I'm sorry I'm behind on reviewing pull requests. This one seems to be urgent, so I'll make this high priority.
@toxigenicpoem where we left off was a discussion on whether to try to shim up the current behavior and add this as a patch, or to declare it as a breaking change, and increment the version number. My feeling is that given how the behavior of the underlying file transport library is a bit different, it might be wise to declare a breaking change. However if we decided to do this, we should lump in any other breaking API changes into one release, so that we're responsibly following semver and not bumping the major version too often. I was hoping to hear from @pifantastic on this, but maybe you can sub in with your thoughts.
IMHO this should be a major version change, but this is definitely not ready for release at this point:
- headers option doesn't work
- encodePaths behavior changed
Maybe other stuff I haven't used is broken too - the core functionality: upload/download is solid - I've been using it in production for 2+ months
I am not aware of any other breaking changes that are waiting to be merged.
