fix(apigateway): move url property to RestApiBase #27742

hariprakash-j · 2023-10-29T10:16:01Z

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

aws-cdk-automation

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.

hariprakash-j · 2023-10-29T10:19:02Z

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.

sumupitchayan · 2023-10-30T17:30:05Z

@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

hariprakash-j · 2023-10-31T14:08:08Z

@sumupitchayan Thanks for the help with the integration tests and I added the integ tests and the unit tests.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

hariprakash-j · 2023-10-31T16:28:24Z

@sumupitchayan I was able to find a aws account with free tier to run the integ tests and generate the snapshots

sumupitchayan · 2023-11-02T18:32:10Z

packages/aws-cdk-lib/aws-cloudfront-origins/lib/rest-api-origin.ts

@@ -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 = {}) {


@hariprakash-j Can you explain the reasoning behind this change and why it is necessary?

@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.

@sumupitchayan , let me know if I'm missing something here

sumupitchayan · 2023-11-07T22:26:48Z

packages/aws-cdk-lib/aws-apigateway/test/restapi.test.ts

-        '/',
-        { Ref: 'apiDeploymentStageprod896C8101' },
-        '/']],
+        ['',


What are these new space changes? If this was a formatting change can you remove this?

@sumupitchayan , changing it back shows a eslint error. it seems they did not indent this well earlier.

@sumupitchayan , do you think i should do something here ?

mergify · 2023-11-30T17:45:33Z

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 · 2023-11-30T17:49:18Z

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).

Pull request has been modified.

mergify · 2023-12-05T00:10:45Z

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).

vinayak-kukreja · 2023-12-05T00:13:37Z

@Mergifyio update

mergify · 2023-12-05T00:13:41Z

update

✅ Branch has been successfully updated

Pull request has been modified.

vinayak-kukreja · 2023-12-05T02:52:37Z

@Mergifyio update

mergify · 2023-12-05T02:52:42Z

update

✅ Branch has been successfully updated

mergify · 2023-12-05T02:52:44Z

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 · 2023-12-05T03:20:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: bad5b43
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-05T03:20:55Z

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*

fix: move url property to RestApiBase

0c88ca9

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Oct 29, 2023

aws-cdk-automation requested a review from a team October 29, 2023 10:16

mergify bot mentioned this pull request Oct 29, 2023

fix(apigateway): move url property to RestApiBase #27741

Closed

aws-cdk-automation previously requested changes Oct 29, 2023

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Oct 29, 2023

mikewrighton assigned sumupitchayan Oct 30, 2023

hariprakash-j added 2 commits October 31, 2023 19:27

Merge branch 'aws:main' into fix(apigateway)/restapi-url-property-fix

e5eb3d8

feat: added integ and unit tests

c008f5b

hariprakash-j added 5 commits October 31, 2023 19:43

Merge branch 'main' into fix(apigateway)/restapi-url-property-fix

f6f015c

fix: lint

a3aaf5d

fix: json in integ test

16ce365

fix: openapi json for tests

caf54ec

feat: add snapshots for new tests

d929bbb

github-actions bot mentioned this pull request Nov 1, 2023

Monthly PR metrics report - October 2023 #27786

Closed

hariprakash-j mentioned this pull request Nov 1, 2023

Cloudfront Origin does not support SpecRestApi #27501

Closed

sumupitchayan reviewed Nov 2, 2023

View reviewed changes

hariprakash-j requested a review from sumupitchayan November 6, 2023 19:36

sumupitchayan reviewed Nov 7, 2023

View reviewed changes

hariprakash-j requested a review from sumupitchayan November 27, 2023 05:49

sumupitchayan previously approved these changes Nov 30, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 30, 2023

Merge branch 'main' into fix(apigateway)/restapi-url-property-fix

05346f8

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 4, 2023

vinayak-kukreja previously approved these changes Dec 5, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2023

Merge branch 'main' into fix(apigateway)/restapi-url-property-fix

444b415

Merge branch 'main' into fix(apigateway)/restapi-url-property-fix

2599828

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2023

vinayak-kukreja approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'main' into fix(apigateway)/restapi-url-property-fix

bad5b43

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2023

mergify bot merged commit b23ee1b into aws:main Dec 5, 2023
12 checks passed

hariprakash-j deleted the fix(apigateway)/restapi-url-property-fix branch December 4, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apigateway): move url property to RestApiBase #27742

fix(apigateway): move url property to RestApiBase #27742

hariprakash-j commented Oct 29, 2023

aws-cdk-automation left a comment

hariprakash-j commented Oct 29, 2023

sumupitchayan commented Oct 30, 2023

hariprakash-j commented Oct 31, 2023

hariprakash-j commented Oct 31, 2023

sumupitchayan Nov 2, 2023

hariprakash-j Nov 2, 2023 •

edited

Loading

hariprakash-j Nov 6, 2023

sumupitchayan Nov 7, 2023

hariprakash-j Nov 8, 2023

hariprakash-j Nov 20, 2023

mergify bot commented Nov 30, 2023

mergify bot commented Nov 30, 2023

mergify bot commented Dec 5, 2023

vinayak-kukreja commented Dec 5, 2023

mergify bot commented Dec 5, 2023

vinayak-kukreja commented Dec 5, 2023

mergify bot commented Dec 5, 2023

mergify bot commented Dec 5, 2023

aws-cdk-automation commented Dec 5, 2023

mergify bot commented Dec 5, 2023

fix(apigateway): move url property to RestApiBase #27742

fix(apigateway): move url property to RestApiBase #27742

Conversation

hariprakash-j commented Oct 29, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

hariprakash-j commented Oct 29, 2023

sumupitchayan commented Oct 30, 2023

hariprakash-j commented Oct 31, 2023

hariprakash-j commented Oct 31, 2023

sumupitchayan Nov 2, 2023

Choose a reason for hiding this comment

hariprakash-j Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

hariprakash-j Nov 6, 2023

Choose a reason for hiding this comment

sumupitchayan Nov 7, 2023

Choose a reason for hiding this comment

hariprakash-j Nov 8, 2023

Choose a reason for hiding this comment

hariprakash-j Nov 20, 2023

Choose a reason for hiding this comment

mergify bot commented Nov 30, 2023

mergify bot commented Nov 30, 2023

mergify bot commented Dec 5, 2023

vinayak-kukreja commented Dec 5, 2023

mergify bot commented Dec 5, 2023

✅ Branch has been successfully updated

vinayak-kukreja commented Dec 5, 2023

mergify bot commented Dec 5, 2023

✅ Branch has been successfully updated

mergify bot commented Dec 5, 2023

aws-cdk-automation commented Dec 5, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 5, 2023

hariprakash-j Nov 2, 2023 •

edited

Loading