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

Use the AWS SDK for JavaScript V3 #1851

Closed
wants to merge 10 commits into from
Closed

Conversation

manavg26
Copy link

@manavg26 manavg26 commented May 24, 2023

migrated the code to the AWS SDK for JavaScript V3

Tag: manavg261840-aws-sdk-JSv3

Fixes #1840

@kelson42 kelson42 changed the title manavg26/#1840 Use the AWS SDK for JavaScript V3 May 24, 2023
@kelson42 kelson42 self-requested a review May 24, 2023 17:58
@kelson42
Copy link
Collaborator

@manavg26 Thank you for your PR, look to the detailed log of the CI to understand why it fails. AFAIK there is at least linting problem. Look the command and start the linting locally and push when this is fixed.

@kelson42 kelson42 marked this pull request as draft May 24, 2023 17:59
@manavg26 manavg26 marked this pull request as ready for review May 24, 2023 19:27
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.17 ⚠️

Comparison is base (8bbde9d) 71.42% compared to head (bbac8f2) 71.26%.

❗ Current head bbac8f2 differs from pull request most recent head ff6f3d0. Consider uploading reports for the commit ff6f3d0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1851      +/-   ##
==========================================
- Coverage   71.42%   71.26%   -0.17%     
==========================================
  Files          24       24              
  Lines        2625     2631       +6     
  Branches      595      594       -1     
==========================================
  Hits         1875     1875              
- Misses        644      652       +8     
+ Partials      106      104       -2     
Impacted Files Coverage Δ
src/S3.ts 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass of review

package.json Outdated Show resolved Hide resolved
src/S3.ts Outdated Show resolved Hide resolved
src/S3.ts Outdated Show resolved Hide resolved
@kelson42
Copy link
Collaborator

@manavg26 For me the feature is broken, if I launch MWoffliner with S3 configuration I get:

**********

Unable to connect to S3, either S3 login credentials are wrong or bucket cannot be found
                            Bucket used: xxxxx
                            End point used: xxxx
                            Public IP used: [object Promise]

**********

@kelson42
Copy link
Collaborator

@manavg26 what about my last comment that it does not work for me (without th pr it works)? Do i need to change some5ing in my configuration?

@manavg26
Copy link
Author

@manavg26 what about my last comment that it does not work for me (without th pr it works)? Do i need to change some5ing in my configuration?

Have made some changes, can you check now?

@manavg26 manavg26 requested a review from kelson42 May 30, 2023 17:01
@kelson42
Copy link
Collaborator

@manavg26 Still same failure. Do you have tested your code? With which ObjectStorage provider? To me if I read the error reporting it seems pretty clear what is the error. You see "Public IP used: [object Promise]" so it seems we have at least some kind of problem related to the IP detection.

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still failing

@kelson42
Copy link
Collaborator

kelson42 commented Jul 18, 2023

Transfering ownership of issue/PR revieing to @VadimKovalenkoSNF. At some point this has to be fixed/reviewed so we can avoid warning and soon outage because of S3 backend.

Maybe this PR needs to be superseeded.

secretAccessKey: this.params.secretAccessKey,
s3ForcePathStyle: s3UrlBase.protocol === 'http:',
const s3UrlObj: any = new URL(this.url)
const regionRegex: RegExp = /^https?:\/\/s3\.([^.]+)\.amazonaws\.com/;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazonaws is not a valid sub-domain for test purposes, there should be wasabisys instead. Also, It should be configurable and not present in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, please fix PR.

return true
}
} catch (err) {
throw new Error(`Unable to connect to S3, either S3 login credentials are wrong or bucket cannot be found
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is not explicit enough, probably err.message should be used instead.

@kelson42 kelson42 mentioned this pull request Jul 21, 2023
@kelson42
Copy link
Collaborator

superseeded by #1865

@kelson42 kelson42 closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade aws-sdk version
3 participants