Conversation
835ca2a to
36b13dc
Compare
d5f1d80 to
b5d37e9
Compare
|
@bentsku i would say it's OK to break persistence in master for now! |
Okay thanks! I'll refactor this PR to remove the utils to access the old version of the store, and will only use the new format instead to access resources. Thanks! |
b5d37e9 to
2e49038
Compare
eb02f3e to
deb89d3
Compare
calvernaz
left a comment
There was a problem hiding this comment.
Great job, love to see code gone 👍
| response = rest_api.to_dict() | ||
| remove_empty_attributes_from_rest_api(response) | ||
| store = get_apigateway_store(account_id=context.account_id, region=context.region) | ||
| store.rest_apis[request["restApiId"]].rest_api = response |
There was a problem hiding this comment.
could we wrap this into store.add_rest_api(api_id, response) same for delete ?
| children = rest_api_resources.setdefault(parent_id, []) | ||
| children.append(response["id"]) | ||
| # FIXME: delete conditional before v2 | ||
| if rest_api := store.rest_apis.get(rest_api_id): |
There was a problem hiding this comment.
my initial comment as also related to abstract the store operations like this. Not sure if possible, but pretty much abstract the internals of the store into methods that can be refactored further without changing callers code in the future.
There was a problem hiding this comment.
It's a bit difficult, as the error message will be different depending on the operation. It's hard to abstract that away.
I've dealt with a provider implementing a lot of logic into the store, and it made it really hard to maintain, because after a while you need to implement operation specific behaviour into the store logic.
But that is actually a leftover, and should be deleted, so thanks for pointing out 😄 (realised I hadn't push the cleanup of all these conditionals)
| return {} | ||
|
|
||
|
|
||
| # ----------------------- |
| """ | ||
| tags = tags or {} | ||
| result = fn(self, *args, tags=tags, **kwargs) | ||
| # TODO: lower the custom_id when getting it from the tags, as AWS is case insensitive |
There was a problem hiding this comment.
AWS doesn't have this concept of custom_id, this is a feature localstack has to facilitate lookups. You probably mean tags in general are case-insensitive.
I do have a general sentiment, we should make these lookups case-insensitive but keep the original form. So logically would never fail (because of the casing) but keep visuals as the author intended.
There was a problem hiding this comment.
https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html
check the tag keys example and the spreadsheets headers.
There was a problem hiding this comment.
Yes, the issue is, the ID will always be lowercase when part of the host. That's why I thought we should force the restApiId to be lower case, even though the user specified a cased tag. (tag could be cased, but not the ID once used to create the restApi).
Even though it's a LocalStack feature, a regular restApiId is always lowercase, and AWS will fail if you upper case an existing ID. Making it case insensitive changes that behaviour.
We could also simply raise an error if the user provided tag value is not lowercase for __custom_id__ or something.
My general idea is just that restApiId should be lowercase. There are no changes yet in behaviour anyway, as I keep it in a CaseInsentiveDict, but it makes the lookup more complicated for no real gain on our side.
ddff601 to
80e8476
Compare
80e8476 to
d1da5a4
Compare
|
Successful PR run against Pro: https://github.com/localstack/localstack/actions/runs/4341060270 |
This PR refactors the store of API Gateway to make use of dictionary for an easier look up of RestAPI sub-entities.
I've added utilities to support the use of both old restored stores and new, fresh stores, but it makes it a bit verbose.This refactor will also help in the recent parity work, to move more data into our stores and add more validations.
I'm not sure how much breakage we can do with persistence, but if we can break, I'll remove the utils and just use the new store attributes directly, which will be much cleaner. If we can't break it yet, we can keep those utils to support restored state, and remove them before
v2. I will be working on API Gateway until then, so I can undertake that effort then.Also cleaned up some methods that were not used anymore anywhere.
\cc @whummer @thrau about how this should tackled, should I break the store, or use these utils until then?
note: green lighted the breakage of persistence, so now this PR just uses the new store format 😄