-
Notifications
You must be signed in to change notification settings - Fork 159
fix: bundle lambdas in script instead of command line #655
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #655 +/- ##
===========================================
+ Coverage 90.78% 95.65% +4.87%
===========================================
Files 8 4 -4
Lines 282 207 -75
Branches 52 30 -22
===========================================
- Hits 256 198 -58
+ Misses 25 9 -16
+ Partials 1 0 -1
Continue to review full report at Codecov.
|
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.
Looks good! Could you please add the link of the failed GitHub action run to provide context for this fix?
ensureDirectoryExistence(`${outputDir}\\${fileToMove}`); | ||
fs.copyFileSync(`${inputDir}\\${fileToMove}`, `${outputDir}\\${fileToMove}`); | ||
} |
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.
nit - No new line at end of file
…n-aws-deployment into cdk-lambdaBundleFix
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.
Looks good! One last thing on logging -
@@ -67,8 +66,9 @@ export default class RestHookHandler { | |||
event: SQSEvent, | |||
allowListPromise: Promise<{ [key: string]: AllowListInfo }>, | |||
): Promise<SQSBatchResponse> { | |||
await ensureAsyncInit(allowListPromise); | |||
console.log(allowListPromise); |
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.
Use logger.debug
instead? The allowList might contain sensitive information that customers do not want in the log all the time.
): Promise<SQSBatchResponse> { | ||
await ensureAsyncInit(allowListPromise); | ||
logger.debug(allowListPromise); | ||
const allowList = await allowListPromise; | ||
logger.debug(allowList); | ||
const messages = event.Records.map((record: any): SubscriptionNotification => { |
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.
looks like you are no longer calling
await ensureAsyncInit(allowListPromise);
did you mean for that?
`cp ${inputDir}\\bulkExport\\schema\\transitiveReferenceParams.json ${outputDir}\\bulkExport\\schema\\transitiveReferenceParams.json`, | ||
`cp ${inputDir}\\bulkExport\\schema\\${PATIENT_COMPARTMENT_V3} ${outputDir}\\bulkExport\\schema\\${PATIENT_COMPARTMENT_V3}`, | ||
`cp ${inputDir}\\bulkExport\\schema\\${PATIENT_COMPARTMENT_V4} ${outputDir}\\bulkExport\\schema\\${PATIENT_COMPARTMENT_V4}`, | ||
`node scripts/build_lambda.js ${inputDir} ${outputDir} bulkExport\\glueScripts\\export-script.py`, |
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.
I think we will want forward slashes not back slashes - https://stackoverflow.com/a/38428899
Issue #, if available:
Description of changes:
Created a script to move files as needed instead of using command line hooks. This resolves the failing action here https://github.com/awslabs/fhir-works-on-aws-deployment/actions/runs/2678017744
Checklist:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.