-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Api Gateway resource functionality enhancements #3121
Api Gateway resource functionality enhancements #3121
Conversation
bclements
commented
Nov 8, 2018
- Added API Gateway resource Integrations functionality: Filter, Update, Delete. Looks like my filter class is only returning the top-level resource integrations and not the child resources. Will fix shortly.
- Added tests for API Gateway resource integrations filter functionality. Will add update and delete tests shortly.
- Fixed updated-method action setting permissions to apigateway:PATCH instead of apigateway:GET
ci failing on bad doctoring ReST formatting
|
Got it and yes I copied and pasted since the logic was similar. Code reuse
and all that, Imitation is a form of flattery, etc. :) Wasn’t happy with
the code I committed anyways so I have already rewritten it this weekend to
work how I wanted. I’ll modify to add in your suggestion and recommit with
working tests shortly.
…On Sat, Nov 10, 2018 at 7:15 AM Kapil Thangavelu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In c7n/resources/apigw.py
<#3121 (comment)>
:
> +
+ return results
+
+ def process_task_set(self, client, task_set):
+ results = []
+ for r, m in task_set:
+ try:
+ integration = client.get_integration(
+ restApiId=r['restApiId'],
+ resourceId=r['id'],
+ httpMethod=m)
+ except client.exceptions.NotFoundException:
+ continue
+
+ integration.pop('ResponseMetadata', None)
+ integration['restApiId'] = r['restApiId']
I understand this implementation is copy/pasted from the rest-method impl,
but the preference would be to have these annotations on the method
flyweights/resources with the c7n: prefix
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3121 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADoj3bk6xnDymaukeQqOPo_ytiHdilltks5uttFYgaJpZM4YVpp1>
.
|
@kapilt Can you merge master into this branch so tests can run on my fixes to test_apigw.py ? I'm trying to do the final tests to get this PR closed out. |
so this was recorded with a gw that has one integration on a single method, but four methods present on the api, which means alot of the recorded files are also just not found responses, it would be better to have a minimal api with one of each, which cut down on the recorded output here significantly. |
@kapilt Please review when you get a moment and let me know if there are any additional changes preventing master merge. Thanks! |
…ix commit corruption.
c714219
to
7e3ce19
Compare
thanks for cleaning up the recorded output, one minor else lgtm |
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, thanks
found one more minor re redefining rest api class for vpc link resource |
…ceintegration-enhancement