added apis for route53resovler #6673
Conversation
There was a problem hiding this comment.
Thanks for the PR @macnev2013 Could you please rebase to master (it should drop the S3 failing tests)?
| return resolver_query_log_config | ||
|
|
||
|
|
||
| def get_resolver_query_log_config_associations(id): |
There was a problem hiding this comment.
nit: I would just add some type hints to these utility functions
There was a problem hiding this comment.
Great set of changes @macnev2013 ! 🚀 Only a minor comment related to attaching the util functions to the backend - would be great if we could add this before merging (this will make some future refactorings easier for us to handle 👍 ).
| @@ -106,3 +114,72 @@ def delete_firewall_rule(firewall_rule_group_id, firewall_domain_list_id): | |||
| firewall_rule = get_firewall_rule(firewall_rule_group_id, firewall_domain_list_id) | |||
| region_details.firewall_rules.get(firewall_rule_group_id, {}).pop(firewall_domain_list_id) | |||
| return firewall_rule | |||
|
|
|||
|
|
|||
| def get_resolver_query_log_config(id): | |||
There was a problem hiding this comment.
It would be preferable to attach these util functions to the backend directly, i.e.:
class Route53ResolverBackend(RegionBackend):
...
def get_resolver_query_log_config(self, id):
...
This will help us to unify the access to the backends, and avoid having too many calls to Route53ResolverBackend.get() spread over the codebase.. (i.e., you would then access self.resolver_query_log_configs instead of region_details.resolver_query_log_configs, etc.)
Can we make this change for the new util functions below, as well as the existing ones above?
There was a problem hiding this comment.
Chiming in here. I think this was due to a previous suggestion of mine since I wanted to keep the backends as lean as possible to avoid picking issues. But as discussed with @whummer, this should not be a big issue.
There was a problem hiding this comment.
Ah, ok good point @giograno . Either way is fine - we can also fix that up in a follow-up iteration, if and when we make the switch to unify the backend access protocol.. 👍
There was a problem hiding this comment.
Hey, It would be a clean approach to keep the backend related operation in the same class. I've updated it.
| return resolver_query_log_config | ||
|
|
||
|
|
||
| def delete_resolver_query_log_config(id): |
There was a problem hiding this comment.
nit: Inside this method we could call get_resolver_query_log_config(id) to assert the existence of the query log config with the given id - the ResourceNotFoundException will be raised from there directly. This would help us reduce the duplicate checks and exceptions a bit.
| if not firewall_rule_group: | ||
| raise ResourceNotFoundException( | ||
| f"Can't find the resource with ID '{id}'. Trace Id: '{aws_stack.get_trace_id()}'" | ||
| self.resolver_query_log_configs = {} |
There was a problem hiding this comment.
nit: One more small comment - would be great if we could add type hints (and a short description) for these member variables, similar to what we have for firewall_rule_groups in line 15 above.
We could also pick that up in one of the next PRs @macnev2013 .. 👍
API Added