-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Remove legacy lambda provider #9543
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
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 0m 35s ⏱️ - 3m 15s Results for commit 88193c6. ± Comparison against base commit 541593a. ♻️ This comment has been updated with latest results. |
6dd6b1b to
7b9e2b4
Compare
4f48606 to
912103d
Compare
| "This feature is marked for removal. Please use AWS client API to seed Kinesis streams.", | ||
| ), | ||
| EnvVarDeprecation( | ||
| "PROVIDER_OVERRIDE_LAMBDA", |
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.
Adding the provider override as a deprecation option helps users cleaning up their configuration
| "KMS_PROVIDER", | ||
| "LAMBDA_DOWNLOAD_AWS_LAYERS", | ||
| # Irrelevant post v3 but intentionally tracked for some time | ||
| "LAMBDA_EXECUTOR", |
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.
It might be interesting to know how many users still have legacy variables configured post v3 🤷
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.
can't hurt and we can still turn it off later 👍
| @@ -0,0 +1,11 @@ | |||
| # TODO: remove this legacy construct when re-working event source mapping. | |||
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.
The event source mapping should be reworked post v3 but I tried to keep this removal PR minimal and avoid functional changes.
| ).sqs | ||
|
|
||
|
|
||
| # TODO: remove once DLQ handling is refactored following the removal of the legacy lambda provider |
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.
The DLQ utilility should be reworked post v3 but I tried to keep this removal PR minimal and avoid functional changes.
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 choice 😁
|
|
||
| # FIXME remove usage of this config value with 2.0 | ||
| @patch.object(config, "SYNCHRONOUS_KINESIS_EVENTS", False) | ||
| # TODO: is this test relevant for the new provider without patching SYNCHRONOUS_KINESIS_EVENTS? |
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.
❓ Is this test still relevant?
- It appears AWS-validated but breaking
- The patching of
SYNCHRONOUS_KINESIS_EVENTSwas only relevant for the old provider
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.
prob not, but let's defer that until later
fb5da7d to
99278b3
Compare
7850f27 to
7494c20
Compare
dominikschubert
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.
LGTM!
Very thorough removal of nearly everything from the legacy provider! Awesome 🚀
|
|
||
| # FIXME remove usage of this config value with 2.0 | ||
| @patch.object(config, "SYNCHRONOUS_KINESIS_EVENTS", False) | ||
| # TODO: is this test relevant for the new provider without patching SYNCHRONOUS_KINESIS_EVENTS? |
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.
prob not, but let's defer that until later
| ).sqs | ||
|
|
||
|
|
||
| # TODO: remove once DLQ handling is refactored following the removal of the legacy lambda provider |
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 choice 😁
| "KMS_PROVIDER", | ||
| "LAMBDA_DOWNLOAD_AWS_LAYERS", | ||
| # Irrelevant post v3 but intentionally tracked for some time | ||
| "LAMBDA_EXECUTOR", |
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.
can't hurt and we can still turn it off later 👍
dfangl
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.
LGTM! All those removed lines 🤩
We still need to keep them in the `CONFIG_ENV_VARS` if we still want deprecation messages to be shown in the LocalStack container.
7494c20 to
88193c6
Compare
Companion PR: https://github.com/localstack/localstack-ext/pull/2296
Motivation
Following the deprecation of the old Lambda provider in v2.0, we can finally get rid of the old Lambda provider in v3.0.
Changes
legacypackages in Move lambda v1 to legacy package #9473Notes
LegacyInvocationResult, which has been re-named and preserved until we re-work event source mappings (e.g., thesqs_event_source_listenerstill depends on theLegacyInvocationResultand the adapers can be removed post v3).LegacyInvocationException, which has been re-named and preserved until we re-work the DQL util_send_to_dead_letter_queue.Discussion
localstack/runtime/analytics.pyDo we keep the analytics of deprecated variables post 3.0?