fix(apigateway): move url property to RestApiBase #27742
Conversation
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Exemption Request I just moved the property to the base class, I am not sure what tests are required for this. If there is any thing that I can do for this please let me know. |
aws-cdk-automation
added
pr-linter/exemption-requested
|
@hariprakash-j Can you add a unit test for your use case? Additionally, this needs to have an Integ test for the use case you are trying to solve. I can run the integ test if you write it |
|
@sumupitchayan Thanks for the help with the integration tests and I added the integ tests and the unit tests. |
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@sumupitchayan I was able to find a aws account with free tier to run the integ tests and generate the snapshots |
| @@ -35,7 +35,7 @@ export interface RestApiOriginProps extends cloudfront.OriginProps { | |||
| */ | |||
| export class RestApiOrigin extends cloudfront.OriginBase { | |||
|
|
|||
| constructor(restApi: apigateway.RestApi, private readonly props: RestApiOriginProps = {}) { | |||
| constructor(restApi: apigateway.RestApiBase, private readonly props: RestApiOriginProps = {}) { | |||
There was a problem hiding this comment.
@hariprakash-j Can you explain the reasoning behind this change and why it is necessary?
There was a problem hiding this comment.
@sumupitchayan Earlier the url property was only in the class RestApi and the classes that extended it. The SpecRestApi class did not inherit from this class as it extended RestApiBase and thus caused an error when one tried to use it a SpecRestApi with RestApiOrigin . Now, however you can use any class that extends the RestApiBase since i have moved the url property to the to this class. This will fix the the issue that was outlined in the issue and prevent this from happening in any new RestApi construct as they will all inherit from the RestApiBase or a class that extends it.
There was a problem hiding this comment.
@sumupitchayan , let me know if I'm missing something here
| '/', | ||
| { Ref: 'apiDeploymentStageprod896C8101' }, | ||
| '/']], | ||
| ['', |
There was a problem hiding this comment.
What are these new space changes? If this was a formatting change can you remove this?
There was a problem hiding this comment.
@sumupitchayan , changing it back shows a eslint error. it seems they did not indent this well earlier.
There was a problem hiding this comment.
@sumupitchayan , do you think i should do something here ?
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
mergify
bot
dismissed
sumupitchayan’s stale review
Pull request has been modified.
aws-cdk-automation
added
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
@Mergifyio update |
✅ Branch has been successfully updated |
mergify
bot
dismissed
vinayak-kukreja’s stale review
Pull request has been modified.
aws-cdk-automation
added
the
pr/needs-maintainer-review
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Moved the url property from RestApi class to the RestApiBase to have the url property on all derived class. This will fix the problem mentioned in the issue where SpecRestApi does not work with the RestApiOrigin construct. PS. I could not perform the integration tests as the free tier on my personal account expired. Closes aws#27501. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Moved the url property from RestApi class to the RestApiBase to have the url property on all derived class. This will fix the problem mentioned in the issue where SpecRestApi does not work with the RestApiOrigin construct.
PS. I could not perform the integration tests as the free tier on my personal account expired.
Closes #27501.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license