8000 Fix put_targets validation regressions by joe4dev · Pull Request #11903 · localstack/localstack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix put_targets validation regressions #11903

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 22, 2024
Merged

Fix put_targets validation regressions #11903

merged 2 commits into from
Nov 22, 2024

Conversation

joe4dev
Copy link
Member
@joe4dev joe4dev commented Nov 22, 2024 •
edited
Loading

Motivation

put_targets does not validate invalid inputs for target_id, which is a regression compared to EventBridge v1.

Changes

  • Add validation exceptions for put_targets

@joe4dev joe4dev added the semver: patch Non-breaking changes which can be included in patch releases label Nov 22, 2024
@joe4dev joe4dev added this to the 4.0.1 milestone Nov 22, 2024
@joe4dev joe4dev self-assigned this Nov 22, 2024
@github-actions GitHub Actions
Copy link
github-actions bot commented Nov 22, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 2m 35s ⏱️ - 40m 11s
2 029 tests  - 1 539  1 913 ✅  - 1 322  116 💤  - 217  0 ❌ ±0 
2 031 runs   - 1 539  1 913 ✅  - 1 322  118 💤  - 217  0 ❌ ±0 

Results for commit deab555. ± Comparison against base commit 7c2d1e31.

This pull request removes 1539 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@joe4dev joe4dev marked this pull request as ready for review November 22, 2024 10:48
@joe4dev joe4dev requested a review from maxhoheiser as a code owner November 22, 2024 10:48
dfangl
dfangl approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member
@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@joe4dev joe4dev merged commit bdcb1dc into master Nov 22, 2024
35 checks passed
@joe4dev joe4dev deleted the fix-put-targets-validation-error branch November 22, 2024 14:16
bentsku
bentsku reviewed Nov 22, 2024
View reviewed changes
localstack-core/localstack/services/events/provider.py
@@ -1152,7 +1153,16 @@ def put_targets(
failed_entries = rule_service.add_targets(targets)
rule_arn = rule_service.arn
rule_name = rule_service.rule.name
for target in targets: # TODO only add successful targets
for index, target in enumerate(targets): # TODO only add successful targets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

came too late, but python tip: enumerate takes a start argument, avoiding this index+1 issue

for index, target in enumerate(targets, start=1):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is really cool, didn't know that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfangl dfangl dfangl approved these changes

@bentsku bentsku bentsku left review comments

@maxhoheiser maxhoheiser Awaiting requested review from maxhoheiser maxhoheiser is a code owner

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees

@joe4dev joe4dev

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@joe4dev @dfangl @bentsku
0