Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

EventBridge: Multi-accounts compatibility#9023

Merged
viren-nadkarni merged 24 commits into
masterfrom
eventbridge-multi-accounts
Sep 14, 2023
Merged

EventBridge: Multi-accounts compatibility#9023
viren-nadkarni merged 24 commits into
masterfrom
eventbridge-multi-accounts

Conversation

@viren-nadkarni
Copy link
Copy Markdown
Member

@viren-nadkarni viren-nadkarni commented Aug 30, 2023

Motivation

When using values other than 000000000000 for account ID or us-east-1 for region, EventBridge would still create the consequent resources in this accounts and region.

Changes

  • Use the correct account ID and region from the request context
  • Remove uses of get_account_id() and get_region() helpers
  • Make explicit use of test account IDs and region names in consturction of ARNs and queue URLs

@viren-nadkarni viren-nadkarni added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Aug 30, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 30, 2023

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 11m 55s ⏱️ - 43m 41s
2 206 tests +3  1 709 ✔️ +17  497 💤 ±0  0  - 14 
2 207 runs  +3  1 709 ✔️ +17  498 💤 ±0  0  - 14 

Results for commit f62aa28. ± Comparison against base commit fa10d1d.

This pull request removes 3 and adds 6 tests. Note that renamed tests count towards both.
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_into_event_bus
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_with_target_sns
tests.aws.services.events.test_events.TestEvents ‑ test_trigger_event_on_ssm_change
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_into_event_bus[domain]
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_into_event_bus[path]
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_with_target_sns[domain]
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_with_target_sns[path]
tests.aws.services.events.test_events.TestEvents ‑ test_trigger_event_on_ssm_change[domain]
tests.aws.services.events.test_events.TestEvents ‑ test_trigger_event_on_ssm_change[path]

♻️ This comment has been updated with latest results.

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 31, 2023

Coverage Status

coverage: 80.122% (+0.001%) from 80.121% when pulling f62aa28 on eventbridge-multi-accounts into fa10d1d on master.

@viren-nadkarni viren-nadkarni force-pushed the eventbridge-multi-accounts branch from d13f419 to 3b3ad9f Compare September 11, 2023 14:00
@viren-nadkarni viren-nadkarni force-pushed the eventbridge-multi-accounts branch from 3b3ad9f to d3678d3 Compare September 12, 2023 06:05
enabled_api_keys = []
if not client:
client = connect_to(region_name=region_name).apigateway
client = connect_to().apigateway
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Temporary change until the client arg is made mandatory


return _get_arn


Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There was a fixture and a utility (localstack.utils.aws.arns.sqs_queue_arn) with the same name that caused confusion and silent overwrite in some instances. The fixture is removed in favour of the utility.

@viren-nadkarni viren-nadkarni force-pushed the eventbridge-multi-accounts branch from 2321cc2 to 86d2b5d Compare September 12, 2023 07:24
@viren-nadkarni viren-nadkarni marked this pull request as ready for review September 12, 2023 08:28
Copy link
Copy Markdown
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, though there"s a lot more than eventbridge in there :D

thanks for tacking this

Comment on lines +1299 to +1301
@pytest.mark.parametrize("strategy", ["domain", "path"])
def test_trigger_event_on_ssm_change(self, monkeypatch, aws_client, clean_up, strategy):
monkeypatch.setattr(config, "SQS_ENDPOINT_STRATEGY", strategy)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@viren-nadkarni viren-nadkarni force-pushed the eventbridge-multi-accounts branch from 8be0c83 to 46c55e7 Compare September 14, 2023 06:40
@viren-nadkarni
Copy link
Copy Markdown
Member Author

The fixture has been restored due to concerns from Daniel that it will hinder the ability to run all tests against AWS. It's been renamed to sqs_get_queue_arn

@viren-nadkarni viren-nadkarni merged commit 0cdaf55 into master Sep 14, 2023
@viren-nadkarni viren-nadkarni deleted the eventbridge-multi-accounts branch September 14, 2023 11:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants