-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
pin botocore with ASF API updates, add ASF API forward compatibility #10183
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 11s ⏱️ Results for commit fc24ad8. ♻️ This comment has been updated with latest results. |
|
The last step - updating the service provider implementations after the API superclasses were updated - would have meant to iterate over hundreds of operations manually. In order to avoid this, I wrote a small script which uses import inspect
from collections import defaultdict
from pathlib import Path
from types import FunctionType
import libcst
from localstack.testing.aws.asf_utils import collect_implemented_provider_operations
# collect all provider operations
provider_ops = collect_implemented_provider_operations()
# iterate over all operations and group all operations which need to be modified per provider file
operations_per_provider_file: dict[str, list[str]] = defaultdict(list)
for provider_class, base_class, provider_function in provider_ops:
try:
sub_function = getattr(provider_class, provider_function)
except AttributeError:
raise AttributeError(
f"Given method name ('{provider_function}') is not a method of the sub class ('{provider_class.__name__}')."
)
if not isinstance(sub_function, FunctionType):
raise AttributeError(
f"Given method name ('{provider_function}') is not a method of the sub class ('{provider_class.__name__}')."
)
if not getattr(sub_function, "expand_parameters", True):
# if the operation on the subclass has the "expand_parameters" attribute (it has a handler decorator) set to False, we don't care
continue
if wrapped := getattr(sub_function, "__wrapped__", False):
# if the operation on the subclass has a decorator, unwrap it
sub_function = wrapped
operations_per_provider_file[inspect.getfile(provider_class)].append(sub_function.__name__)
class ProviderMethodTransformer(libcst.CSTTransformer):
def __init__(self, operations: list[str]):
self.operations = operations
super().__init__()
def leave_FunctionDef(
self, original_node: libcst.FunctionDef, updated_node: libcst.FunctionDef
) -> libcst.CSTNode:
if original_node.name.value in self.operations and not original_node.params.star_kwarg:
print(f" -> {original_node.name.value}")
new_params = updated_node.params.with_changes(
star_kwarg=libcst.Param(name=libcst.Name("kwargs"))
)
return updated_node.with_changes(params=new_params)
return updated_node
# iterate over all providers and tranform the found operations
for provider_file, ops in operations_per_provider_file.items():
print(f"- {provider_file}")
module = libcst.parse_module(Path(provider_file).read_bytes())
updated_module = module.visit(ProviderMethodTransformer(ops))
Path(provider_file).write_text(updated_module.code) |
9c156f1 to
ecf10b1
Compare
LocalStack Community integration with Pro 2 files 2 suites 1h 21m 24s ⏱️ Results for commit fc24ad8. ♻️ This comment has been updated with latest results. |
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.
Changes look great! Just before we go this route: have you thought about solving this on the dispatcher level, i.e., filtering the kwargs in the Skeleton based on the handler's signature, so superfluous arguments don't even make it into the method call?
That would limit the necessary code changes. And I'm worried that we will become lazy with updating providers, now that we also no longer get warnings in the IDE that signatures between API stubs and providers no longer match, and drift will become worse over time.
|
Good question, and I thought about this a lot actually. I would argue that:
|
134c2f9 to
7665953
Compare
7665953 to
5cfc33f
Compare
thrau
left a comment
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.
well clearly you thought about it so let's go with this solution :-) other than that i don't like the look of the trailing **kwargsattribute in every method, i don't have a strong argument against it right now. the runtime overhead of filtering a dict would be negligible though.
anyway, great work on the overall changes!
5cfc33f to
fc24ad8
Compare
Motivation
This PR addresses multiple issues around the ASF API updates.
In the past we had issues several times because a
botocoreupdate slipped in (botocorewas not pinned), while the ASF APIs have not been updated (they only run every Monday morning).This can then cause issues in the fallback forwarding due to our strict APIs:
botocorepackage.TypeError.The last instance of this issue occurred during our 3.1 release: CircleCI Job # 175308
This PR aims at solving this issue with two different approaches at once:
botocoreand only update it together with the generated APIs.**kwargsto all ASF API definitions and implementations.botocore. In these projects we cannot pinbotocore, because it's transitively pinned thoughlocalstack-core.This PR is basically an extension of #10127 (now that we have proper dependency pinning, explicitly ping botocore and tie it to the ASF updates).
Changes
The changes are clearly separated in different commits:
**kwargsto all generated API operations.botocorein thesetup.cfgto the version which was last used to update the ASF APIs (Update ASF APIs #10173).botocore.Testing