Skip to content

Bugfix/eventbridge/event bus desciption not asigned on create #12100

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 5 commits into from
Jan 9, 2025
Merged

Bugfix/eventbridge/event bus desciption not asigned on create #12100

merged 5 commits into from
Jan 9, 2025

Conversation

maxhoheiser
Copy link
Member

@maxhoheiser maxhoheiser commented Jan 3, 2025 •
edited
Loading

Motivation

Event Bridge Buses can have a description, this description is returned on create and on describe event buses.

This PR fixes the issue raised in this github bug report: #12065

Changes

Correctly store and handle the description of an event bridge bus on create.

Testing

Add test for creating and describing event bridge bus with description

@maxhoheiser maxhoheiser added aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Jan 3, 2025
@maxhoheiser maxhoheiser self-assigned this Jan 3, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 8m 34s ⏱️ - 46m 58s
2 385 tests  - 1 558  2 288 ✅  - 1 346  97 💤  - 212  0 ❌ ±0 
2 387 runs   - 1 558  2 288 ✅  - 1 346  99 💤  - 212  0 ❌ ±0 

Results for commit f74b6e1. ± Comparison against base commit 0191993.

This pull request removes 1562 and adds 4 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[False-regions0]
tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[False-regions1]
tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[True-regions0]
tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[True-regions1]

♻️ This comment has been updated with latest results.

@maxhoheiser maxhoheiser force-pushed the bugfix/eventbridge/event-bus-desciption-not-asigned-on-create branch from b0e6e74 to 2658814 Compare January 8, 2025 15:04
@maxhoheiser maxhoheiser marked this pull request as ready for review January 8, 2025 15:06
@maxhoheiser maxhoheiser requested a review from joe4dev as a code owner January 8, 2025 15:06
@maxhoheiser maxhoheiser requested a review from bentsku January 8, 2025 15:06
bentsku
bentsku reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Neat fix! This is great, and good thinking with the parametrization of the test!

I have a comment about accessing the model attribute like that, as we can restore persisted state that might not have the field yet. Using getattr can avoid weird errors popping up and not break existing users' setup.

Once this is addressed, I think we merge the PR. Nice fix! 🚀

edit: looking at the test failures, there might be rebasing issues with the snapshot

localstack-core/localstack/services/events/event_bus.py
@@ -45,6 +46,7 @@ def create_event_bus_service(
region,
account_id,
event_source_name,
description,
Copy link
Contributor

@bentsku bentsku Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

as a note, I think it would be good to use keyword arguments here, it's much cleaner and readable (named parameters), as the order could be quite fragile

localstack-core/localstack/services/events/provider.py Outdated
Comment on lines 966 to 967
if description := event_bus_service.event_bus.description:
response["Description"] = description
Copy link
Contributor

Choose a reason for hiding this comment

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

in the case of the data being restored from persisted state, I'm not sure it's fully possible but the description field could be non-existent. Until the next minor release, it could be good to use getattr(event_bus_service.event_bus, "description", None) to be on the safe side.

one example of a PR who removed such things: #11809 (and the PR implementing one: #11358, see this comment: https://github.com/localstack/localstack/pull/11358/files#r1717458334)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

bentsku
bentsku reviewed Jan 8, 2025
View reviewed changes
localstack-core/localstack/services/events/provider.py Outdated
Comment on lines 1952 to 1953
if event_bus.description:
event_bus_api_type["Description"] = event_bus.description
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still the access here, sorry I didn't put a comment 😅

@maxhoheiser maxhoheiser force-pushed the bugfix/eventbridge/event-bus-desciption-not-asigned-on-create branch from 05b268d to f74b6e1 Compare January 8, 2025 15:45
bentsku
bentsku approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! A small comment for the getattr usage could be good, with a TODO, to know to revisit and remove once we switch for example major versions, but a quick search will do too! Thanks for addressing the comments!

@maxhoheiser
Copy link
Member Author

LGTM! A small comment for the getattr usage could be good, with a TODO, to know to revisit and remove once we switch for example major versions, but a quick search will do too! Thanks for addressing the comments!

I will track it as a todo in the servicepage.

@maxhoheiser maxhoheiser merged commit ec18ec9 into master Jan 9, 2025
31 checks passed
@maxhoheiser maxhoheiser deleted the bugfix/eventbridge/event-bus-desciption-not-asigned-on-create branch January 9, 2025 08:50
@maxhoheiser maxhoheiser mentioned this pull request Jan 9, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

Assignees

@maxhoheiser maxhoheiser

Labels
aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxhoheiser @bentsku