-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(apigateway): add mode
property for SpecRestApi
#34198
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
Conversation
I can't resolve CodeCov error... Run actions/github-script@v7
Error: Error message: Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable
at OidcClient.<anonymous> (/home/runner/work/_actions/actions/github-script/v7/dist/index.js:585:23)
at Generator.next (<anonymous>)
at /home/runner/work/_actions/actions/github-script/v7/dist/index.js:522:71
at new Promise (<anonymous>)
at __webpack_modules__.8041.__awaiter (/home/runner/work/_actions/actions/github-script/v7/dist/index.js:518:12)
at OidcClient.getIDToken (/home/runner/work/_actions/actions/github-script/v7/dist/index.js:571:16)
at Object.<anonymous> (/home/runner/work/_actions/actions/github-script/v7/dist/index.js:421:46)
at Generator.next (<anonymous>)
at /home/runner/work/_actions/actions/github-script/v7/dist/index.js:133:71
at new Promise (<anonymous>)
Error: Unhandled error: Error: Error message: Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable |
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.
Thanks for the PR! Left some minor comments.
/** | ||
* This property applies only when you use OpenAPI to define your REST API. | ||
* The Mode determines how API Gateway handles resource updates. | ||
* |
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.
Shouldn't this property (mode
) be in SpecRestApiProps
?
|
||
---- | ||
|
||
This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project. |
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.
Why is this necessary? (The same statement is above.)
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.
Oops, It was a simple mistake.
export interface RestApiProps extends RestApiOptions { | ||
export interface RestApiProps extends RestApiBaseProps, ResourceOptions { |
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.
This is an appropriate change. However, it may not be the right thing to do with this PR that adds the mode property. As a type, it should be safe since it is compatible with the original type, but we might want to avoid the change just in case in this PR.
* Specifies how API Gateway handles resource updates when importing an OpenAPI definition. | ||
* This property applies only when you use OpenAPI to define your REST API. | ||
*/ | ||
export enum RestApiMode { |
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.
It would be good to bring it below export enum EndpointType
.
test('mode property is set correctly', () => { | ||
// WHEN | ||
const apiWithOverwrite = new apigw.RestApi(stack, 'api-overwrite', { | ||
mode: apigw.RestApiMode.OVERWRITE, | ||
}); | ||
apiWithOverwrite.root.addMethod('GET'); | ||
|
||
const apiWithMerge = new apigw.RestApi(stack, 'api-merge', { | ||
mode: apigw.RestApiMode.MERGE, | ||
}); | ||
apiWithMerge.root.addMethod('GET'); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | ||
Name: 'api-overwrite', | ||
Mode: 'overwrite', | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | ||
Name: 'api-merge', | ||
Mode: 'merge', | ||
}); | ||
}); | ||
|
||
test('mode property is optional', () => { | ||
// WHEN | ||
const api = new apigw.RestApi(stack, 'api'); | ||
api.root.addMethod('GET'); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | ||
Name: 'api', | ||
}); | ||
// Mode should not be present in the template when not specified | ||
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | ||
Mode: Match.absent(), | ||
}); | ||
}); |
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.
We can change like this:
test('mode property is set correctly', () => { | |
// WHEN | |
const apiWithOverwrite = new apigw.RestApi(stack, 'api-overwrite', { | |
mode: apigw.RestApiMode.OVERWRITE, | |
}); | |
apiWithOverwrite.root.addMethod('GET'); | |
const apiWithMerge = new apigw.RestApi(stack, 'api-merge', { | |
mode: apigw.RestApiMode.MERGE, | |
}); | |
apiWithMerge.root.addMethod('GET'); | |
// THEN | |
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | |
Name: 'api-overwrite', | |
Mode: 'overwrite', | |
}); | |
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | |
Name: 'api-merge', | |
Mode: 'merge', | |
}); | |
}); | |
test('mode property is optional', () => { | |
// WHEN | |
const api = new apigw.RestApi(stack, 'api'); | |
api.root.addMethod('GET'); | |
// THEN | |
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | |
Name: 'api', | |
}); | |
// Mode should not be present in the template when not specified | |
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | |
Mode: Match.absent(), | |
}); | |
}); | |
test.each([ | |
[apigw.RestApiMode.OVERWRITE, 'overwrite'], | |
[apigw.RestApiMode.MERGE, 'merge'], | |
[undefined, undefined], | |
])('mode property is set (%s)', (mode, expectedMode) => { | |
// WHEN | |
const api = new apigw.RestApi(stack, 'api', { | |
mode, | |
}); | |
api.root.addMethod('GET'); | |
// THEN | |
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', { | |
Name: 'api', | |
Mode: expectedMode ?? Match.absent(), | |
}); | |
}); |
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.
Thanks for the suggestion!
The PR has been merged, but still seems to have that problem. (I've seen that problem in other PRs as well.) In this PR, it's good to not worry about it for now. |
@@ -66,6 +70,24 @@ book.addMethod('GET'); | |||
book.addMethod('DELETE'); | |||
``` | |||
|
|||
When using OpenAPI to define your REST API, you can control how API Gateway handles resource updates using the `mode` property. Valid values are: |
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 this property is applied when using OpenAPI only, it would be good to bring it in the ### OpenAPI Definition
, what do you think?
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.
I think so, moved this contents to ## OpenAPI Definition
section 🙋🏻♂️
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Thanks for the changes. I'll approve after applying the last comments.
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
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.
LGTM.
It might be good to simplify the PR title, like: feat(apigateway): add `mode` property for SpecRestApi
.
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.
Sorry, There was something I had missed...
You can control how API Gateway handles resource updates using the `mode` property. Valid values are: | ||
|
||
* `overwrite` - The new API definition replaces the existing one. The existing API identifier remains unchanged. | ||
* `merge` - The new API definition is merged with the existing API. | ||
|
||
If you don't specify this property, a default value is chosen: | ||
* For REST APIs created before March 29, 2021, the default is `overwrite` | ||
* For REST APIs created after March 29, 2021, the new API definition takes precedence, but any container types such as endpoint configurations and binary media types are merged with the existing API. | ||
|
||
Use the default mode to define top-level RestApi properties in addition to using OpenAPI. | ||
Generally, it's preferred to use API Gateway's OpenAPI extensions to model these properties. | ||
|
||
```ts | ||
const api = new apigateway.SpecRestApi(this, 'books-api', { | ||
apiDefinition: apigateway.ApiDefinition.fromAsset('path-to-file.json'), | ||
mode: apigateway.RestApiMode.MERGE | ||
}); | ||
``` |
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.
Sorry. I noticed that the description should be below the following statement:
**Note:** When starting off with an OpenAPI definition using `SpecRestApi`, it is not possible to configure some
properties that can be configured directly in the OpenAPI specification file. This is to prevent people duplication
of these properties and potential confusion.
And it might be good to change the following sentence to be more natural... (Because the description is related to the above statements, or will be in conflict.)
You can control how API Gateway handles resource updates using the `mode` property. Valid values are:
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.
I fixed. Added However,
.
mode
propertyfor how API Gateway resources are updated when using an OpenAPI definitionmode
propertyto SpecRestApi
😂 (Or |
mode
propertyto SpecRestApi
mode
property to SpecRestApi
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
I think it's because you're typing in double-byte (full-width) characters. So copy and paste this as is. (And it is changed with
|
mode
property to SpecRestApi
mode
property for SpecRestApi
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.
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). |
This pull request has been removed from the queue for the following reason: The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
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 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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
N/A
Reason for this change
AWS::ApiGateway::RestApi
has amode
property, but the L2 construct for API Gateway (SpecRestApi class) does not currently expose this property.Therefore, I added the
mode
property to theSpecRestApiProps
interface.Description of changes
mode
property inSpecRestApiProps
.RestApiMode
to define the possible values formode
. This enum includes two values:merge
andoverwrite
.Describe any new or updated permissions being added
No new IAM permissions are introduced.
Description of how you validated changes
packages/aws-cdk-lib/aws-apigateway/test/restapi.test.ts
packages/@aws-cdk-testing/framework-integ/test/aws-apigateway/test/integ.spec-restapi.ts
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.