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
Implementation of Lambda Advanced Configuration attribute #10591
Conversation
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 1h 32m 0s ⏱️ - 1m 25s Results for commit 034461c. ± Comparison against base commit f6c21e3. This pull request removes 1 and adds 7 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
# Conflicts: # tests/aws/services/lambda_/test_lambda_api.snapshot.json # tests/aws/services/lambda_/test_lambda_api.validation.json
tests.aws.services.lambda_.test_lambda_api.TestLambdaLayer.test_layer_function_exceptions
|
tests.aws.services.lambda_.test_lambda_api.TestLambdaLayer.test_layer_function_exceptions
…ion from snapshot testing
Following test fails on AWS:
|
Following test is failing on AWS:
|
# Conflicts: # tests/aws/services/lambda_/test_lambda_api.snapshot.json
localstack/tests/aws/services/lambda_/test_lambda.py::TestLambdaConcurrency::test_lambda_provisioned_concurrency_scheduling
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 done @Morijarti with detailed test coverage 👏👏👏 LGTM 👍
I just added some minor suggestions to improve the naming, then everything is ready to merge 🚀
For further runtime updates (including ext), it is much better to tackle them in a separate PR.
"HTTPHeaders": {}, | ||
"HTTPStatusCode": 200 | ||
} | ||
"AllowedPublishers": { |
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.
@dfangl FYI: AWS apparently changed the code signing behavior (see this backlog item)
@@ -13839,44 +14439,82 @@ | |||
} | |||
} | |||
}, | |||
"update-during-in-progress-update-exc": { |
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.
snapshot diffs are often so confusing 😆
@Morijarti I triggered an ext test run against this branch. Please double-check that this one has no failing Lambda tests before merging, thank you! |
@joe4dev I've updated PR with your suggestions. Thank you so much :) |
I found few lambda related tests crashing in -ext. I'll create a companion PR for them. |
} | ||
} | ||
}, | ||
"tests/aws/services/lambda_/test_lambda_api.py::TestLambdaFunction::test_function_partial_advanced_logging_configuration_update[partial_config0]": { |
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.
@Morijarti Can you remove these old snapshots after the renaming? Unfortunately, we currently don't have a convenient way to clean up such old snapshot leftovers 🧹
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.
Sure thing
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.
Done
self, snapshot, create_lambda_function, lambda_su_role, aws_client | ||
): | ||
function_name = f"fn-{short_uid()}" | ||
create_response = create_lambda_function( |
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.
I just noticed that we don't have a test case where we initialize the logging configuration with the function creation. Would it make sense to add that here, given we are testing the default in the partial update below?
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.
Good catch.
I can add new tests for that.
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.
Feel free to add it here directly. It feels we have already quite extensive API test coverage for a single parameter.
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.
Good catch, we weren't actually modifying logging config if passed during function creation.
It should be fixed now
@@ -869,6 +869,33 @@ def create_function( | |||
) | |||
# Runtime management controls are not available when providing a custom image | |||
runtime_version_config = None | |||
if "LoggingConfig" in request: |
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.
glad we caught that one 😃
Motivation
At the moment, AWS support Lambda Advanced configuration, which is caussing issue with snapshot testing since we do not support it in LS.
Changes
This PR aims to add advanced config to outputs in order to remove various ignores from tests