Lambda refactoring/CRUD ASF provider implementation #6865
Conversation
dominikschubert
force-pushed
the
lambda-crud-newprovider
branch
from
f3f2fab
to
bddfbf7
Compare
| def __init__(self, *args, **kwargs): | ||
| super(ServiceException, self).__init__(*args) | ||
|
|
||
| if len(args) >= 1: | ||
| self.message = args[0] | ||
| else: | ||
| self.message = "" | ||
| for key, value in kwargs.items(): | ||
| setattr(self, key, value) |
There was a problem hiding this comment.
i approve this change :-) coincidentally had this thought here: #6875 (comment)
@bentsku will also make your life easier
…ecessary gradlew wrappers
…or get_alias on non existent one
dfangl
force-pushed
the
lambda-crud-newprovider
branch
from
a01e2ea
to
78771e3
Compare
There was a problem hiding this comment.
Wohooo, fantastic set of changes, 🚀 🚀 extremely excited to see this evolving! Like the approach with frozen dataclasses - immutable state will avoid lots of headaches we've had the past. 👌 Really clean data model and encapsulation of logic for the different aspects of the API, great job.
Also, kudos for the huge depth of testing, insane number of new snapshot tests, which really give us high confidence of parity. 🥳
Added a few minor suggestions and a few questions - mostly nitpicks, nothing major. Looking forward to making this the default soon.. 🙂
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class VersionFunctionConfiguration: | ||
| # fields | ||
| # name: str | ||
| function_arn: str # TODO:? |
There was a problem hiding this comment.
Out of curiosity, what's the required change (TODO) here?
There was a problem hiding this comment.
@dominikschubert do you remember? I think we wanted to remove it, as the ID including the arn is saved in the FunctionVersion objects?
There was a problem hiding this comment.
this is a pretty massive PR, congrats on these changes, the impeccable testing and the high-quality code. it's easy to read and understand. this PR is i think also a good demonstration of how verbosity can be good thing, and that code golf is not the way we should develop providers.
💯 🏅
my comments are mostly cosmetic changes and one related to IO that we can probably fix later.
| s3_client: "S3Client" = aws_stack.connect_to_service("s3", region_name="us-east-1") | ||
| kwargs = {"VersionId": self.s3_object_version} if self.s3_object_version else {} | ||
| try: | ||
| s3_client.delete_object(Bucket=self.s3_bucket, Key=self.s3_key, **kwargs) |
There was a problem hiding this comment.
are we forcing us-east-1 when creating s3 code objects as well? otherwise delete_object may raise an exception, right?
There was a problem hiding this comment.
Yes, all our internal bucket storage is currently in us-east-1. As s3 is global, does not make too many differences, but for the sake of locationconstraints etc, its easier. At some point we want to hide the s3 bucket (its currently enumerable by the user), for example by defining a "LocalStack account". That would also enable us to use s3 storage for other services (e.g. ECR), making persistence and file storage easier.
| security_group_ids: List[str] = dataclasses.field(default_factory=list) | ||
| subnet_ids: List[str] = dataclasses.field(default_factory=list) | ||
| security_group_ids: list[str] = dataclasses.field(default_factory=list) | ||
| subnet_ids: list[str] = dataclasses.field(default_factory=list) |
There was a problem hiding this comment.
just curios: what in your opinion is better in using the list builtin compared to the List from typing?
There was a problem hiding this comment.
@dominikschubert you have a stronger opinion there. In my opinion its just easier, as you do not need an import.
There was a problem hiding this comment.
Yeah I'm generally pro list/dict etc. I see no reason to use List over list when we don't have to support earlier python versions.
There was a problem hiding this comment.
my argument for List and Dict would be that it's more consistent when using other imports from typing, like Optional, Type, Generic, etc...
Goal
Continued implementation for new ASF provider for lambda. The goal of this PR is to make the integration tests in
tests.integration.awslambda/test_lambda_api.py|test_lambda.py|test_lambda_common.pypass for the new provider on CircleCI since this was disabled in the preceding test rework.Changes
Future Work