feat(apigatewayv2): http api - domain url for a stage #15973
Conversation
|
|
peterwoodworth
added
@aws-cdk/aws-apigatewayv2
| /** | ||
| * API domain name | ||
| */ | ||
| public readonly domainName?: string; |
There was a problem hiding this comment.
This should not be optional.
Why not just set the type to IDomainName instead?
There was a problem hiding this comment.
Good idea, I've updated the type here
…rror if _addDomainMapping() is called more than once, and renamed customDomainUrl to domainUrl
| @@ -45,7 +50,10 @@ export abstract class StageBase extends Resource implements IStage { | |||
| * @internal | |||
| */ | |||
| protected _addDomainMapping(domainMapping: DomainMappingOptions) { | |||
| new ApiMapping(this, `${domainMapping.domainName}${domainMapping.mappingKey}`, { | |||
| if (this.apiMapping) { | |||
| throw new Error('_addDomainMapping cannot be called more than once for a Stage'); | |||
There was a problem hiding this comment.
| throw new Error('_addDomainMapping cannot be called more than once for a Stage'); | |
| throw new Error('Only one ApiMapping allowed per Stage'); |
There was a problem hiding this comment.
@nija-at What is the recommended pattern for testing a protected function?
There was a problem hiding this comment.
I see the problem, it might be a bit convoluted.
I think it's fine to skip the test case for this. Sorry for the mixed message.
There was a problem hiding this comment.
Not a problem! Thanks for the response
| if (!this.apiMapping.domainName.name) { | ||
| throw new Error('Unable to build domainUrl due to invalid domainName'); | ||
| } |
There was a problem hiding this comment.
Shouldn't need this anymore now that domainName is not optional, right?
If the check is for domainName being empty string (indicated by your test case),
(a) I wonder if empty string domain names are valid strings. Perhaps disallow it in the CDK?
(b) update the message to be more clear, i.e., what is invalid about it.
There was a problem hiding this comment.
It doesn't appear that the aws cli command for creating an API Mapping allows for an empty string after some testing.
Should I update the ApiMapping or DomainName constructor to throw this error?
There was a problem hiding this comment.
Yes, that would be preferrable.
There was a problem hiding this comment.
@nija-at I had updated my comment perhaps after you read it. Which class constructor should I put this error in? ApiMapping or DomainName?
DomainName would catch the error earlier, but I want to check with you before I proceed.
There was a problem hiding this comment.
DomainName sounds like the right spot.
nija-at
changed the title
|
Could you also make an entry in the README as indicated by the linter? |
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
…ssary test case from stage test
Done. Please let me know if there is any further detail I need to provide here |
|
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). |
Closes aws#15801 By adding and `apiMapping` field to base.ts, we can reference the custom domain information from the `HttpStage` construct ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Closes aws#15801 By adding and `apiMapping` field to base.ts, we can reference the custom domain information from the `HttpStage` construct ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Closes #15801
By adding and
apiMappingfield to base.ts, we can reference the custom domain information from theHttpStageconstructBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license