-
Notifications
You must be signed in to change notification settings - Fork 0
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
PRMP-935 new lambda-generate-stitch-record #199
Conversation
|
||
module "generate-stitch-record-lambda" { | ||
source = "./modules/lambda" | ||
name = "GenerateStitchRecord" |
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.
Can we call this GenerateLloydGeorgeStitch
? So there is some consistency between this and the LloydGeorgeStitch
lambda? Also it's a bit hard to tell which is responsible for creating the stitch job and Record
is hard to differentiate between dynamo and patient records
infrastructure/dynamo_db.tf
Outdated
@@ -134,6 +134,38 @@ module "zip_store_reference_dynamodb_table" { | |||
owner = var.owner | |||
} | |||
|
|||
module "stitch_store_reference_dynamodb_table" { |
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.
Can we change this name also, as there's isn't a Stitch Store
bucket. Maybe LloydGeorgeStitchJobMetadata
?
@@ -72,19 +72,21 @@ module "delete-doc-ref-lambda" { | |||
module.ndr-lloyd-george-store.s3_object_access_policy, | |||
"arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole", | |||
"arn:aws:iam::aws:policy/CloudWatchLambdaInsightsExecutionRolePolicy", | |||
module.ndr-app-config.app_config_policy_arn | |||
module.ndr-app-config.app_config_policy_arn, | |||
module.stitch_metadata_reference_dynamodb_table.dynamodb_policy |
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.
Are we missing a depends on for these?
@@ -55,7 +55,7 @@ module "generate-document-manifest-lambda" { | |||
"arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole", | |||
"arn:aws:iam::aws:policy/CloudWatchLambdaInsightsExecutionRolePolicy", | |||
module.ndr-app-config.app_config_policy_arn, | |||
aws_iam_policy.dynamodb_stream_policy.arn | |||
aws_iam_policy.dynamodb_stream_manifest_policy.arn |
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.
Missing a depends_on
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.
Why does it need to be in depends_on
??
infrastructure/dynamo_db.tf
Outdated
source = "./modules/dynamo_db" | ||
table_name = var.stitch_metadata_dynamodb_table_name | ||
hash_key = "ID" | ||
deletion_protection_enabled = false |
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.
Probably should be true
?
@@ -101,6 +103,7 @@ module "create-doc-ref-lambda" { | |||
module.create-doc-ref-gateway, | |||
module.ndr-app-config, | |||
module.lloyd_george_reference_dynamodb_table, | |||
module.document_reference_dynamodb_table | |||
module.document_reference_dynamodb_table, | |||
module.stitch_metadata_reference_dynamodb_table, |
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.
trailing comma
] | ||
} | ||
|
||
resource "aws_iam_policy" "dynamodb_stream_policy" { | ||
resource "aws_iam_policy" "dynamodb_stream_manifest_policy" { |
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.
If we're renaming this already, can it be called dynamodb_stream_manifest
instead?
APPCONFIG_APPLICATION = module.ndr-app-config.app_config_application_id | ||
APPCONFIG_ENVIRONMENT = module.ndr-app-config.app_config_environment_id | ||
APPCONFIG_CONFIGURATION = module.ndr-app-config.app_config_configuration_profile_id | ||
LLOYD_GEORGE_BUCKET_NAME = "${terraform.workspace}-${var.lloyd_george_bucket_name}" |
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.
Hyphen?
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.
Thanks for making the changes :).
No description provided.