Skip to content

Refactor EventBridge ArchiveServices to allow restoration without side effects #11891

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 1 commit into from
Nov 22, 2024
Merged

Refactor EventBridge ArchiveServices to allow restoration without side effects #11891

merged 1 commit into from
Nov 22, 2024

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Nov 21, 2024 •
edited
Loading

Motivation

Currently, ArchiveServices have quite some side effects in its constructor.

This prevents us from avoiding those side effects when we want to restore those services from data in the store (for cloudpods or persistence).

With this PR, we split out the logic out of the constructor, to allow instantiation without side effects.

Changes

  • Remove boto calls from the ArchiveService constructor
  • Refactor a bit to manually trigger rule/target registration

@dfangl dfangl added this to the 4.1 milestone Nov 21, 2024
@dfangl dfangl added the semver: patch Non-breaking changes which can be included in patch releases label Nov 21, 2024
@dfangl dfangl modified the milestones: 4.1, 4.0.1 Nov 21, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 21, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 2m 12s ⏱️ - 40m 34s
2 029 tests  - 1 539  1 912 ✅  - 1 323  117 💤  - 216  0 ❌ ±0 
2 031 runs   - 1 539  1 912 ✅  - 1 323  119 💤  - 216  0 ❌ ±0 

Results for commit 75f7a01. ± 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.

@dfangl dfangl force-pushed the events/more-persistence-fixes branch from a390c5a to 75f7a01 Compare November 22, 2024 11:00
@dfangl dfangl changed the title Add more fixes for events persistence Refactor EventBridge ArchiveServices to allow restoration without side effects Nov 22, 2024
@dfangl dfangl marked this pull request as ready for review November 22, 2024 11:07
joe4dev
joe4dev approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

LGTM

localstack-core/localstack/services/events/archive.py
"""Creates a target for the archive rule. The target is required for accessing parameters
from the provider during sending of events to the target but it is not invoked
because events are put to the archive directly to not overload the gateway"""
target_id = f"Events-Archive-{self.name}"
self.client.put_targets(
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for separating the initial creation to avoid such boto calls during persistence loading 👍

@dfangl dfangl merged commit e1e028d into master Nov 22, 2024
38 checks passed
@dfangl dfangl deleted the events/more-persistence-fixes branch November 22, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

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

Assignees

@joe4dev joe4dev

@dfangl dfangl

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.
2 participants
@dfangl @joe4dev