Switch url rewriter from js to typescipt #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I pulled in a new library @aws-cdk/aws-lambda-nodejs that handles the transformation of javascript to typescript. Had to patch up a couple of things which made this change a little more complex that intended. - All of the CDK components were not version locked, that meant depending on the order of npm installs they could drift and get into a dependancy mismatch spiral. The version itself isn't important, its uniformity is all that matters. - Added permissions to the lambda role for interacting with cloudfronts distribution system, these were handled by the lambda function during deployment, but it seems this new library while handling the js -> ts brought some limitations and I had to manually implement the permssions see for details https://tinyurl.com/5xy3n8zt - Need to fix some typescript compliation issues, please feel free to re-work all of this and just keep the method signature Signed-off-by: Peter Nied <[email protected]>
|
This resolves opensearch-project#1569 (comment) |
1 task
dblock
reviewed
Feb 8, 2022
|
|
||
| console.log('maxBuildNumber', maxBuildNumber); | ||
| } catch (ex) { | ||
| console.error(ex); |
There was a problem hiding this comment.
We should fail hard on exception here which will cause s 500. Otherwise we will never know we had an error.
There was a problem hiding this comment.
Needed to fix some typescript compliation issues, please feel free to re-work all of this and just keep the method signature
I agree with you recommendation.
For this PR I did the minimal amount of changes I could. The flow of this method still seems under development in the core PR and I'd rather continue the discussion on that review
|
@tianleh I would recommend merging these PRs or implementing the changes you prefer in |
tianleh
approved these changes
Feb 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I pulled in a new library @aws-cdk/aws-lambda-nodejs that handles the
transformation of javascript to typescript. Had to patch up a couple of
things which made this change a little more complex that intended.
All of the CDK components were not version locked, that meant
depending on the order of npm installs they could drift and get into a
dependancy mismatch spiral. The version itself isn't important, its
uniformity is all that matters.
Added permissions to the lambda role for interacting with cloudfronts
distribution system, these were handled by the lambda function during
deployment, but it seems this new library while handling the js -> ts
brought some limitations and I had to manually implement the permssions see
for details https://tinyurl.com/5xy3n8zt
Needed to fix some typescript compliation issues, please feel free to re-work all of this and just keep the method signature
Signed-off-by: Peter Nied [email protected]