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

Support user provided environment variable key for aws-lambda-dynamodb pattern #132

Closed
1 of 2 tasks
beomseoklee opened this issue Feb 15, 2021 · 6 comments
Closed
1 of 2 tasks
Labels
feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged

Comments

@beomseoklee
Copy link
Contributor

Currently, the aws-lambda-dynamodb pattern creates a Lambda environment variable DDB_TABLE_NAME by default, but it would be better if users can set up the default environment key.

Use Case

Users can define the Lambda environment variable key. For example, if the DynamoDB table is for metadata of their services, they can set METADATA_DYNAMODB_TABLE instead of DDB_TABLE_NAME, and it would make more sense when they see the Lambda environment variables on the Lambda console.

Proposed Solution

Maybe,

export interface LambdaToDynamoDBProps {
  ...
  readonly lambdaDynamoTableEnvironmentVariableKey?: string
}

if (props.lambdaDynamoTableEnvironmentVariableKey) {
  this.lambdaFunction.addEnvironment(props.lambdaDynamoTableEnvironmentVariableKey, this.dynamoTable.tableName);
} else {
  this.lambdaFunction.addEnvironment('DDB_TABLE_NAME', this.dynamoTable.tableName);
}

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@beomseoklee beomseoklee added feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged labels Feb 15, 2021
@biffgaut
Copy link
Contributor

Thanks - this is an interesting idea and we'll take a look. It's important to remember that one of the tenets of Solutions Constructs is that they are consistent - that if someone uses one construct with DynamoDB that they know that other constructs with DynamoDB behave the same way (see the guidelines for each service in the DESIGN_GUIDELINES.md file. So this change would change the DESIGN_GUIDELINES.md file and require changing every similar occurrence (although off the top of my head it seems that this is unique to the combination of Lambda and DynamoDB).

@beomseoklee
Copy link
Contributor Author

@biffgaut I've searched addEnvironment() and it seems like they are very general like you mentioned. So, it if can be consistent, maybe all patterns which call addEnvironment() would need to support the custom environment variable key on Lambda function, so let me know your thoughts.

@biffgaut
Copy link
Contributor

We’ll talk about it at our team meeting Monday – seems like a reasonable thing.

This could be labeled a bug, because if you connect a Lambda function to 2 different tables then either one construct will overwrite the value of the other or the stack will crash when the variable names collide. That leads to a possible increase in scope – avoiding collisions.

I would think that providing a name would be optional (if they don’t then we use a default) – so if they attach a function to 2 tables, we could:

  • Change the behavior so that all environment names get a prefix based on Construct ID. This prevents collisions, but will make it impossible to document the variable name – the user will have to figure it out each time.
  • Leave the behavior as is and collide. This sounds harsh, but this hasn’t been seen as a problem to this point and allows us to document the variable name easily. We could also add a warning to the environment variable name parameter documentation explaining the risk.

Let us know your thoughts before Monday - thanks.

@beomseoklee
Copy link
Contributor Author

For me, providing flexibility to change the default environment name would be fine, so if no name is provided, like you mentioned, patterns create the name for users, but if name is provided, patterns use the user provided name. :)

@biffgaut
Copy link
Contributor

biffgaut commented Mar 2, 2021

OK, go ahead and move forward on this. The scope is all constructs where Lambda is accessing a resource (not where Lambda is being triggered), we see these:

lambda-dynamodb
lambda-elasticsearch-kibana
lambda-s3
lambda-sagemakerendpoint
lambda-sns
lambda-sqs
lambda-sqs-lambda
lambda-step-function

The props attributes need to express what the variable represents and stress that is the variable name (to distinguish it from full environment variables which can be specified in the lambda properties), so use:

TableEnvironmentVariableName?: string
QueueEnvironmentVariableName?: string
TopicArnEnvironmentVariableName?: string
TopicNameEnvironmentVariableName?: string
DomainEndpointEnvironmentVariableName?: string
SagamakerEndpointEnvironmentVariableName?: string
etc.

If the property is not specified, use the current default value. Update the README.md files documentation at the bottom like this:

  • Set environment variables:
    • (default) SAGEMAKER_ENDPOINT_NAME
    • AWS_NODEJS_CONNECTION_REUSE_ENABLED (for Node 10.x and higher functions)

I'll update the DESIGN_GUIDELINES.md file.

@beomseoklee
Copy link
Contributor Author

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged
Projects
None yet
Development

No branches or pull requests

2 participants