Add copyObject function to S3Client #53
Conversation
|
|
| @@ -48,10 +48,10 @@ export function s3TestSuite(data) { | |||
| // Assert | |||
| expect(gotFirstObject).to.be.an('object') | |||
| expect(gotFirstObject.key).to.equal(data.s3.testObjects[0].key) | |||
| expect(gotFirstObject.body).to.equal(data.s3.testObjects[0].data) | |||
| expect(gotFirstObject.data).to.equal(data.s3.testObjects[0].body) | |||
There was a problem hiding this comment.
These getObject tests were broken, but they were passing since both values were undefined.
There was a problem hiding this comment.
Good catch indeed 👍🏻 Thanks for fixing it on the way 🙇🏻
| {} | ||
| ) | ||
|
|
||
| const res = http.request(method, signedRequest.url, signedRequest.body || null, { |
There was a problem hiding this comment.
I'm interested to get some additional thoughts on this line. The other methods default the request body to an empty string instead of null. When I default to '', the unit tests pass but I see the following error when using the copyObject in a real k6 test:
ERRO[0002] Uncaught (in promise) S3ServiceError: A header you provided implies functionality that is not implemented
There was a problem hiding this comment.
TL;DR: body || null should be the default from now on, so, you did good 👍🏻
The body || '' is legacy, we just didn't get to fix it yet unfortunately. We're gonna prioritize that. In general, a reason we should prefer to use null instead of '' is that it avoids getting in the way of the signature process (AWS verifies the signature partly based on the hash of the body, and explicitly setting the body was a way for us to make sure that if no body was present, we would calculate the same hash than AWS).
|
Hey @bendennis 🙇🏻 So sorry for coming back to you so late, this PR got completely off my radar, and my email notifications. Thanks a lot for your contribution, I'll take a look now 👀 |
| {} | ||
| ) | ||
|
|
||
| const res = http.request(method, signedRequest.url, signedRequest.body || null, { |
There was a problem hiding this comment.
TL;DR: body || null should be the default from now on, so, you did good 👍🏻
The body || '' is legacy, we just didn't get to fix it yet unfortunately. We're gonna prioritize that. In general, a reason we should prefer to use null instead of '' is that it avoids getting in the way of the signature process (AWS verifies the signature partly based on the hash of the body, and explicitly setting the body was a way for us to make sure that if no body was present, we would calculate the same hash than AWS).
| @@ -48,10 +48,10 @@ export function s3TestSuite(data) { | |||
| // Assert | |||
| expect(gotFirstObject).to.be.an('object') | |||
| expect(gotFirstObject.key).to.equal(data.s3.testObjects[0].key) | |||
| expect(gotFirstObject.body).to.equal(data.s3.testObjects[0].data) | |||
| expect(gotFirstObject.data).to.equal(data.s3.testObjects[0].body) | |||
There was a problem hiding this comment.
Good catch indeed 👍🏻 Thanks for fixing it on the way 🙇🏻
|
Again, great work! 🎉 Thanks a lot for your contribution @bendennis 🙇🏻 I'll merge this and we will release a 0.8.1 or 0.9.0 version as soon as possible. Will ping you in the documentation PR when I open it, so you can double check that my documentation is in line with your interpretation 👍🏻 |
|
Sounds good! Thanks @oleiade! |
|
Here is the documentation for the feature: grafana/k6-docs#1273 |
This PR adds a copyObject function to the S3Client.