Lambda: Fix behaviour for alias update and delete operations #11878
Lambda: Fix behaviour for alias update and delete operations #11878dfangl merged 2 commits intolocalstack:masterfrom
Conversation
anisaoshafi
requested review from
dfangl,
dominikschubert,
gregfurman and
joe4dev
as code owners
|
All contributors have signed the CLA ✍️ ✅ |
localstack-bot
left a comment
localstack-bot
left a comment
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
anisaoshafi
force-pushed
the
issue-11823
branch
from
2a959fb to
74670a0
Compare
dfangl
added
the
semver: patch
dfangl
left a comment
dfangl
left a comment
There was a problem hiding this comment.
This is a great first iteration, thank you! I just have some minor comments and questions. Also, it would be nice to add a quick docstring for every new test which describes the scenario being tested, one line is sufficient. (We try to do that for new tests, although as you see, it is not consistent across the codebase).
| function = self._get_function( | ||
| function_name=function_name, region=region, account_id=account_id | ||
| ) | ||
| fn_arn = api_utils.unqualified_lambda_arn(function_name, account_id, region) |
There was a problem hiding this comment.
There is a utility qualified_lambda_arn as well, where you can specify the alias name as qualifier directly, to avoid manually appending :{name}. Also, we only need to calculate this value in the if block, could you move it there?
There was a problem hiding this comment.
Totally agree, I focused to much on making it work and didn't place it in the right place. Done.
| "tests/aws/services/lambda_/test_lambda_api.py::TestLambdaUrl::test_function_url_config_deletion_without_qualifier": { | ||
| "last_validated_date": "2024-11-19T17:37:22+00:00" | ||
| }, |
There was a problem hiding this comment.
It seems you renamed the test at some point, which leads to these leftovers. Can you (just manually) delete that entry, to keep the validation file tidier?
| self, create_lambda_function_aws, lambda_su_role, snapshot, aws_client | ||
| ): | ||
| function_name = f"alias-fn-{short_uid()}" | ||
| snapshot.add_transformer(SortingTransformer("Aliases", lambda x: x["Name"])) |
There was a problem hiding this comment.
Why is this transformer necessary here?
There was a problem hiding this comment.
copy-pasting 🍝 from a test that worked, it's not needed actually. Thanks for the catch.
| self, create_lambda_function_aws, lambda_su_role, snapshot, aws_client | ||
| ): | ||
| function_name = f"alias-fn-{short_uid()}" | ||
| snapshot.add_transformer(SortingTransformer("Aliases", lambda x: x["Name"])) |
There was a problem hiding this comment.
Same question: Why is this necessary for this test? (as well in the third test)
There was a problem hiding this comment.
copy-pasting 🍝 from a test that worked, it's not needed actually. Thanks for the catch.
dfangl
left a comment
dfangl
left a comment
There was a problem hiding this comment.
Looks good to me now, thank you for addressing my comments! I will not merge it now on purpose (even though it is ready), we will do that together in our next meeting.
Thank you for this contribution!
3 participants
Motivation
As reported in #11832:
update_aliasanddelete_alias, a generic ValueError was thrown which was not in sync with the AWS behavior.delete_function_url_configwith no qualifier, the AWS behavior on the alias fnxs deletion was not clear.It seems:
delete_aliasfor non-existing alias (but a 204 🤷🏼).ResourceNotFoundException( f"Alias not found: {fn_arn}:{name}", Type="User")when updating an alias that doesn't exist (not a ValueError).delete_function_url_config, AWS doesn't delete the function url configs of all aliases, when not specifying the Qualifier.Changes
delete_aliaswhen alias doesn't exist.update_aliaswhen alias doesn't exist.test_non_existent_alias_deletionandtest_non_existent_alias_updatetest_url_config_deletion_without_qualifierthat shows the behaviour on the alias fnxs deletion (doesn't delete the function url configs of all aliases when qualifier not specified)Let me know if I missed something, or if I messed up 🙏🏼