Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(apigateway): rate limited API key #6509
feat(apigateway): rate limited API key #6509
Changes from 6 commits
81f9454
b677b31
4eafcb5
c56bdec
4bccb02
9493c35
dd6269a
6dcce28
05c8742
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 needed? We generally don't encourage any new linter exclusions.
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.
The build was failing after I made RateLimitedApiKeyProps extend ApiKeyProps :
So I followed the suggestion and added
[disable-awslint:ref-via-interface]
to the jsdoc ofresources
property inApiKeyProps
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 see it now.
I believe this is occurring because this object is using
RestApi
instead ofIRestApi
as the type forresources
.Can we try switching it to
IRestApi
and see if all tests still pass?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 tried doing that, but it resulted in compilation errors as
renderStageKeys
method expectsresources
to be of typeRestApi
and changing it toIRestApi
is not possible as it doesn't havedeploymentStage
property needed by this methodhttps://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigateway/lib/api-key.ts#L90-L101
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.
Missed something from last time. We usually don't extend one Props class from another Props class.
The following changes are needed here
ApiKeyProps
toApiKeyOptions
ApiKeyProps
class -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.
Not sure if I understand.
Is the suggestion to :
ApiKeyProps
defined inApiKey
, toApiKeyOptions
ApiKeyProps
) inRateLimitedApiKey
extendingApiKeyOptions
If that's the case, I'm confused how it will help/is better
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.
You're correct. I think I was looking for an optimization/future-proofing whose value is unclear yet. Ignore this.