fix(events): AwsApi warns if service does not exist #13352
Conversation
…xist during synth. Fixes #13090.
|
|
|
Had to add aws-sdk as a dependency (instead of dev dependency) and include it as a bundled dependency to get the build to pass locally. The build and tests pass locally, got no clue about the NOTICE error in the CodeBuild build. |
| @@ -102,8 +101,12 @@ | |||
| "@aws-cdk/aws-stepfunctions": "0.0.0", | |||
| "@aws-cdk/core": "0.0.0", | |||
| "@aws-cdk/custom-resources": "0.0.0", | |||
| "constructs": "^3.2.0" | |||
| "constructs": "^3.2.0", | |||
| "aws-sdk": "^2.848.0" | |||
There was a problem hiding this comment.
I'm not too much of a fan of this.
There are probably just data files we could be copying out of this dependency at build time, no?
In fact, I'm fairly sure there are other modules that are already doing the same. I think probably the AwsCustomResource, which does almost the same as this construct does ?
|
|
||
| const sdkServiceInstance = new sdkService(apiVersion && { apiVersion }); | ||
| if (!sdkServiceInstance[action]) { | ||
| throw new Error(`The action ${action} for the service ${service} does not exist, check the list of available \ |
There was a problem hiding this comment.
The downside of throwing an error is that if our knowledge is incorrect or outdated (for whatever reason, probably because we didn't get the freshest build of the SDK and there's a shiny new API we don't know about yet), the CDK will completely fail to work.
There needs to either be an "I know what I'm doing" override, or this needs to turn into an Annotations.of(this).addWarning().
There was a problem hiding this comment.
AwsApi is not a construct so using Annotations.of(this) doesn't work..
There was a problem hiding this comment.
How about adding the warning to the Lambda handler function?
| let sdkService: any; | ||
| let normalizedServiceName: string; | ||
|
|
||
| if ((SDK as any)[service]) { |
There was a problem hiding this comment.
I'd rather do a complete case-insensitive comparison.
Get a list of all services, map the lowercase name of the service to the actual case, and look up in there.
rix0rrr
changed the title
rix0rrr
changed the title
| * @param service Service name | ||
| */ | ||
| function checkServiceExists(service: string, handler: lambda.SingletonFunction) { | ||
| const sdkService = awsSdkMetadata[service.toLowerCase()]; |
There was a problem hiding this comment.
Are we sure this is correct for services like SQS and such? I'm not too familiar with the contents of this map.
There was a problem hiding this comment.
This map is generated in buildtools/gen.js. It is built from https://github.com/aws/aws-sdk-js/blob/master/apis/metadata.json
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
|
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Check against AWS SDK that the given service and action exist. If a service acronym is supplied check if it needs to be uppercased, e.g. rds to RDS. Fixes aws#13090. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Check against AWS SDK that the given service and action exist. If a service acronym is supplied check if it needs to be uppercased, e.g. rds to RDS. Fixes #13090.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license