-
Notifications
You must be signed in to change notification settings - Fork 23
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
[Step Function] 3.2 Extract class ConfigLoader and LambdaConfigLoader #150
Conversation
serverless/src/lambda/env.ts
Outdated
* addLayers: true | ||
* ... | ||
*/ | ||
public getConfigFromCfnMappings(mappings: any): TConfig { |
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.
serverless/src/lambda/env.ts
Outdated
@@ -86,6 +86,168 @@ export interface Configuration { | |||
apmFlushDeadline?: string; | |||
} | |||
|
|||
abstract class ConfigLoader<TConfig> { |
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 I'm understanding correctly, the stepfunction config loader will (in a future PR) will also extend this class is that right? Do we want it in the lambda folder or is there a shared code section that makes sense to keep the overlapping components in?
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 I'm understanding correctly, the stepfunction config loader will (in a future PR) will also extend this class is that right?
Yes.
Do we want it in the lambda folder or is there a shared code section that makes sense to keep the overlapping components in?
I'm okay with either, but since you raised it, let me move it to the src folder.
* addLayers: true | ||
* ... | ||
*/ | ||
public getConfigFromCfnMappings(mappings: any): TConfig { |
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.
Context of this PR series
Right now the Datadog Serverless CloudFormation Macro only supports instrumenting Lambda functions. This series of PRs makes it also support instrumenting Step Functions.
Instead of merging every PR into main branch, I'm going to merge them into a feature branch
yiming.luo/step-function
to:What does this PR do?
Moves some code for getting Lambda config from top-level functions into two classes:
ConfigLoader
, for common code for loading Lambda config and loading Step Function configLambdaConfigLoader extends ConfigLoader
, for code specific to LambdaMotivation
To avoid duplicate code for loading Lambda config and for loading Step Function config. For example, #148 has lots of duplicate code.
Testing Guidelines
lambda/env.spec.ts
Additional Notes
Types of changes
Check all that apply