-
Notifications
You must be signed in to change notification settings - Fork 273
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
Migrate CfUrlRewriter Lambda from JavaScript to TypeScript #1660
Conversation
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Codecov Report
@@ Coverage Diff @@
## main #1660 +/- ##
============================================
- Coverage 94.69% 94.47% -0.23%
Complexity 14 14
============================================
Files 163 164 +1
Lines 3447 3454 +7
Branches 21 21
============================================
- Hits 3264 3263 -1
- Misses 180 188 +8
Partials 3 3
Continue to review full report at Codecov.
|
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Exclude |
@opensearch-project/engineering-effectiveness for reviews. cc @seraphjiang for visibility |
@gaiksaya Thanks for inviting me to look at this code, I wrote a variant of it in this PR tianleh#4 that you can consult if you are looking for how @tianleh has improved this space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, ship it
@gaiksaya @peterzhu1992 Could you please review? We are trying to wrap up this week. |
Hi @tianleh , looks there is failing ci: https://github.com/opensearch-project/opensearch-build/runs/5341305055?check_suite_focus=true |
Just run the failed cmd locally and it is successful.
|
check more by creating a dummy pr. I can see the failed CI on it #1675 This is indeed strange since this current PR has it successful. |
…pensearch-project#1660)" This reverts commit 6c5887e. Signed-off-by: Tianle Huang <tianleh@amazon.com>
Link to main issue #1492 |
Description
As #1569 grows large, extract the logic of "Migrate CfUrlRewriter Lambda from JavaScript to TypeScript" to this PR. (Thank the previous work tianleh#4 )
This change doesn't not add extra business logic to the Lambda function but just rewrites in TypeScript and move it to its own file. Research about the logic with a few repos. See the related issue to see the complete path to this current PR. #1654
Some highlights of this change:
use the same versions for
@aws-cdk
packages to resolve this type error Migrate CfUrlRewriter Lambda from JavaScript to TypeScript #1654 (comment)introduce the unit test structure for the Lambda function which will be a good foundation for more complicated logic in support latest in distribution build urls #1569
Issues Resolved
Resolves #1654
Test
a) Open without
ci
stringhttps://d17qm4xnop70sv.cloudfront.net/distribution-build-opensearch/2.0.0/1/test.png
b) open with
ci
stringhttps://d17qm4xnop70sv.cloudfront.net/ci/dbc/distribution-build-opensearch/2.0.0/1/test.png
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.