[Multi-bucket] Update options to include bucket #13574
Conversation
… into update/commonOptions
ashika112
requested review from
ukhan-amazon,
haverchuck,
cshfang,
jimblanc,
HuiSF and
a team
as code owners
ashika112
changed the title
| bucket: apiOptions.bucket.bucketName, | ||
| region: apiOptions.bucket.region, |
There was a problem hiding this comment.
Sanity check: doesn't need to validate the object shape?
There was a problem hiding this comment.
I didnt think we checked object structure like that? Do we do this somewhere. I thought usually type is enough.
We do have assertion already if there is no bucket or region in our code that is checked after this piece of code.
| @@ -220,6 +222,70 @@ describe('downloadData with key', () => { | |||
| }), | |||
| ); | |||
| }); | |||
|
|
|||
| describe('bucket passed in options', () => { | |||
There was a problem hiding this comment.
Thanks for adding additional tests.
One thought here is that the bucket info overriding is carried by the function resolveS3ConfigAndInput.ts should we primarily test this function, and verify whether other consumer functions are invoking this function correctly, so we don't need to repeat the same tests in every test suite (as they are basically testing the resolveS3ConfigAndInput function)?
There was a problem hiding this comment.
So couple of pointers here, i think what called out makes sense.
-
I still have not made changes to COPY api which needs resolution in both source and destination. Per design we might end up extracting
resolveBucketConfiglogic out and make it per API level if something pops out from Copy implement so for now i have it top level since that makes most sense. So i wrote the test per APi level so if we make the change it can catch any changes. -
We will still need test APi level here but rather in that case will check if API options is passed right. I think both of these achieve the same result in my mind.
That said, with next PR on copy API if this still stays here i can add in test specific to that function to check high level. But i think it is still beneficial per API level as well. wdyt?
There was a problem hiding this comment.
Not a big deal, but is there a way to compose this such that the same logic can be re-used between suites?
There was a problem hiding this comment.
Yeah i thought about it but we kinda do this in all test suites. Also, i see value in testing this API level like we are doing other places. let me take another look at this. All of these are testing their input on actual s3 call , so i think even if we compose we still need to pass that part in
packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts
Outdated
Show resolved
Hide resolved
| @@ -220,6 +222,70 @@ describe('downloadData with key', () => { | |||
| }), | |||
| ); | |||
| }); | |||
|
|
|||
| describe('bucket passed in options', () => { | |||
There was a problem hiding this comment.
Not a big deal, but is there a way to compose this such that the same logic can be re-used between suites?
Description of changes
bucketDescription of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.