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

Migrate CfUrlRewriter Lambda from JavaScript to TypeScript #1660

Merged
merged 6 commits into from
Feb 26, 2022

Conversation

tianleh
Copy link
Member

@tianleh tianleh commented Feb 21, 2022

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:

  1. use the same versions for @aws-cdk packages to resolve this type error Migrate CfUrlRewriter Lambda from JavaScript to TypeScript #1654 (comment)

  2. 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

  1. add/update unit tests
  2. deploy to my personal AWS account.

a) Open without ci string
https://d17qm4xnop70sv.cloudfront.net/distribution-build-opensearch/2.0.0/1/test.png

b) open with ci string
https://d17qm4xnop70sv.cloudfront.net/ci/dbc/distribution-build-opensearch/2.0.0/1/test.png

Check List

  • Commits are signed per the DCO using --signoff

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.

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
@tianleh tianleh requested a review from a team as a code owner February 21, 2022 02:26
Signed-off-by: Tianle Huang <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2022

Codecov Report

Merging #1660 (40ad871) into main (947bc5d) will decrease coverage by 0.22%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             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              
Impacted Files Coverage Δ
...loyment/lambdas/cf-url-rewriter/cf-url-rewriter.ts 50.00% <50.00%> (ø)
deployment/lib/artifacts-public-access.ts 100.00% <100.00%> (ø)
src/system/temporary_directory.py 83.87% <0.00%> (-12.91%) ⬇️
src/system/os.py 92.30% <0.00%> (-7.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 947bc5d...40ad871. Read the comment docs.

@tianleh
Copy link
Member Author

tianleh commented Feb 21, 2022

Exclude package-lock.json (under deployment) from commits since the js-tests / test will fail by complaining the co-existing of yarn lock and package lock file.

@tianleh
Copy link
Member Author

tianleh commented Feb 23, 2022

@opensearch-project/engineering-effectiveness for reviews.

cc @seraphjiang for visibility

@peternied peternied removed their request for review February 23, 2022 20:40
@peternied
Copy link
Member

peternied commented Feb 23, 2022

@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

Copy link
Member

@seraphjiang seraphjiang left a comment

Choose a reason for hiding this comment

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

LGTM, ship it

@tianleh
Copy link
Member Author

tianleh commented Feb 26, 2022

@gaiksaya @peterzhu1992 Could you please review? We are trying to wrap up this week.

@gaiksaya gaiksaya merged commit 6c5887e into opensearch-project:main Feb 26, 2022
@gaiksaya
Copy link
Member

Hi @tianleh , looks there is failing ci: https://github.com/opensearch-project/opensearch-build/runs/5341305055?check_suite_focus=true
Can you take a look?
Thanks!

@tianleh
Copy link
Member Author

tianleh commented Feb 26, 2022

Just run the failed cmd locally and it is successful.

[opensearch@5981838b19ee deployment]$   yarn test -- --coverage
yarn run v1.22.15
warning package.json: No license field
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ jest --coverage
 PASS  test/lambdas/cf-url-rewriter/cf-url-rewriter.test.ts
Bundling asset MyTestStack/CfUrlRewriter/Code/Stage...

  ...d5e0955a728447d84a7e526c3a33bece7f8f9f183717c03b05cec9602/index.js  747b 

Bundling asset MyTestStack/CfUrlRewriter/Code/Stage...

  ...67fa3d8e9cd6bad9c10dac2623c00a3560ce445c2d385c5b137b7cff2/index.js  747b 

 PASS  test/build-artifact-stack.test.ts
-----------------------------|---------|----------|---------|---------|-------------------
File                         | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------------------|---------|----------|---------|---------|-------------------
All files                    |   94.91 |      100 |   85.71 |   94.91 |                   
 lambdas/cf-url-rewriter     |      50 |      100 |      50 |      50 |                   
  cf-url-rewriter.ts         |      50 |      100 |      50 |      50 | 9-13              
 lib                         |     100 |      100 |     100 |     100 |                   
  artifacts-public-access.ts |     100 |      100 |     100 |     100 |                   
  buckets.ts                 |     100 |      100 |     100 |     100 |                   
  build-artifact-stack.ts    |     100 |      100 |     100 |     100 |                   
  identities.ts              |     100 |      100 |     100 |     100 |                   
-----------------------------|---------|----------|---------|---------|-------------------

Test Suites: 2 passed, 2 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        3.208 s, estimated 5 s
Ran all test suites.
Done in 3.69s.

@tianleh
Copy link
Member Author

tianleh commented Feb 26, 2022

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.

tianleh added a commit to tianleh/opensearch-build that referenced this pull request Feb 26, 2022
@tianleh
Copy link
Member Author

tianleh commented Mar 2, 2022

Link to main issue #1492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate CfUrlRewriter Lambda from JavaScript to TypeScript
5 participants