Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrating to AWS SDK V3 | Use AWS SDK V3 In Unit Tests (s3_ops) #7479

Merged

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Sep 6, 2023

Explain the changes

  1. Implementation of AWS SDK V3 (without wrapper) in unit tests:
    s3_ops
    • Remove request handlers in stage 'error' and 'complete' and use catch error instead.
    • Remove request handlers that added version ID to query params and use the param of the action itself.
    • Replace request handlers with middlewarestack in case of namespace cache.
    • Small fixes:
      • In test 'should version head text-file' - replace the object to match type needed (string, but could be string or Error).
      • In 'test should tag text file' - fix the message value to the right one.

Issues: Fixed #xxx / Gap #xxx

  1. none

Testing Instructions:

  1. make test or make run-single-test testname=test_s3_ops.js (The test runs as a part of the CI)
  • Doc added/updated
  • Tests added

@tangledbytes
Copy link
Member

@shirady I see that in the NPM docs under v2 compatible style they mention:

The client can also send requests using v2 compatible style. However, it results in a bigger bundle size and may be dropped in next major version.

Should we instead use const { S3Client } = require("@aws-sdk/client-s3");?

@shirady
Copy link
Contributor Author

shirady commented Sep 10, 2023

@tangledbytes we can use S3Client instead of S3 in the unit tests, but I thought it might be unclear since we would use it only there. In the other parts of the code, we will keep using AWS SDK V2 because there might be clients that use signature version 2, and we also created a wrapper for AWS SDK V3 in case the client didn't pass the region of the target bucket (here). Since in the unit tests we know that it uses signature version 4, and the region is known I used the AWS SDK V3 directly.

In the introduction (here) under "High-Level Concepts" they show the two options:

  1. Bare-bones clients/commands
  2. Aggregated clients/commands, and it is written:

Under the hood this calls the bare-bones commands.

So by switching to the Bare-bones clients/commands, we would have the same results.
I had an idea to create a wrapper class for each command to use the bare-bones commands while keeping the AWS SDK V2 code style, code snippet for example:

class S3ClientSDKV3BM {
    constructor(params) {
        this.s3 = new S3 Client(params);
    }

    async headObject(params) {
        const res = await this.s3.send(new HeadObjectCommand(params));
        return res;
    }

But it is another part of the code that we would need to maintain - right now we get it from AWS SDK V3.

@tangledbytes
Copy link
Member

@shirady, that makes sense. Let's hope that they don't drop the support in the future 🤞.

@shirady shirady force-pushed the migrate-aws-sdk-imp-unit-tests-part-4 branch 2 times, most recently from 60836d0 to 455afb7 Compare September 19, 2023 06:18
1. s3_ops
   - Remove request handlers in stage 'error' and 'complete' and use catch error instead.
   - Remove request handlers that added version ID to query params and use the param of the action itself.
   - Replace request handlers with middlewarestack in case of namesapce cache.
   - Small fixes:
     - In test 'should version head text-file' - replace object to match type needed (string, but could be string or Error).
     - In 'test should tag text file' - fix the message value to the right one.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady force-pushed the migrate-aws-sdk-imp-unit-tests-part-4 branch from 455afb7 to 3f502ac Compare September 19, 2023 08:47
@shirady shirady merged commit 0e3b958 into noobaa:master Sep 19, 2023
8 checks passed
@shirady shirady deleted the migrate-aws-sdk-imp-unit-tests-part-4 branch September 28, 2023 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants