Conversation
calvernaz
left a comment
There was a problem hiding this comment.
Thanks for doing this, I know working with the split state doesn't make it easier. LGTM 🚀
I have some comments, that can be handle in next iterations
|
|
||
| @staticmethod | ||
| def _remove_rest_api(context: RequestContext, rest_api_id: str) -> None: | ||
| store = get_apigateway_store(account_id=context.account_id, region=context.region) |
There was a problem hiding this comment.
it removes from Localstack store, should we check on Moto's as well ?
There was a problem hiding this comment.
The call_moto should take care of that as the rest API is contained there, I'm not sure how this could happen in moto
|
|
||
|
|
||
| @pytest.fixture | ||
| def apigw_create_rest_resource(apigateway_client): |
There was a problem hiding this comment.
I'm curious about how to create this fixture. Deleting the API container should remove most of the resources created under path, templates, etc - backend integration lingers tho. Any particular use case where this didn't happen? meaning deleting the API wasn't removing resources?
There was a problem hiding this comment.
I'm deleting it, it was me not exactly knowing how it would behave. Thanks!
| def _remove_rest_api(context: RequestContext, rest_api_id: str) -> None: | ||
| store = get_apigateway_store(account_id=context.account_id, region=context.region) | ||
| # clean up this way until we properly encapsulate RestApi in the store | ||
| store.authorizers.pop(rest_api_id, None) |
There was a problem hiding this comment.
Since we want to move the state from moto to localstack, in a next iteration we can functionality to the store to delete an API, basically wrapping a method to all the operation below.
There was a problem hiding this comment.
Right, I'm not sure about adding methods to the store, I've seen it done in some cases, not sure what the proper way of doing it is, any idea @thrau?
There was a problem hiding this comment.
there's something in the old S3 provider which wraps the moto backend with accessor/helper functions. other than that i know of no such patterns.
if you have split brain between moto and localstack stores, and it's a pain, we can look into establishing a pattern.
| # --------------- | ||
|
|
||
|
|
||
| def get_moto_rest_api(context: RequestContext, rest_api_id: str) -> MotoRestAPI: |
There was a problem hiding this comment.
we do have this method self._get_moto_backend(context) on the provider similar to this one. This one seems to handle mis-lookups, should we highlight that and remove usages of the former?
There was a problem hiding this comment.
You're right, the self._get_moto_backend(context) was from my first PR, but not sure it still makes sense now, this new method handles a bit more. If we ever need to access the moto backend to access other kind of resources and not RestAPI, we can create a new method for that resource with the proper exception message. Thanks for picking this up!
835ca2a to
36b13dc
Compare
Work on APIGW Resource parity, improve UpdateResource parity (patch operations validation), remove moto patches to move them into provider directly (we still manipulates moto entities directly as its tightly integrated, but extend moto with our own store), and fix deleting child resources when deleting a resource that was used as a parent.
Example:
/->/pets->/pets/dogsWe delete
/pets,/pets/dogsshould be deleted as well because it declared/petsto be its parent.We do this by mapping children IDs when they are created with a parent ID. When deleting a resource, we check if it is in the parent list, and then recursively delete resources.
UpdateResource(validation)Resourcewhen deleting a parentResourcewithDeleteResourcenote: I might delete the fixture for creating resource in a following PR, as we clean up all related resources when deleting the RestAPI.
note2: more tests are coming for deleting the resource with more children in a following PR. (#7746)
note3: I'd like to do a store refactor to allow easier validation of resources, see #7750
\cc @whummer