-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Eventstudio: Move fixtures to make them available in Pro #12962
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
Eventstudio: Move fixtures to make them available in Pro #12962
Conversation
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.
Nice addition, I just have a few nits and one fixture is not properly cleaning up, once this is addressed we can merge!
if cross_scenario not in ["region", "account", "region_account"]: | ||
raise ValueError(f"cross_scenario {cross_scenario} not supported") |
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.
nit: seems like we are validating twice the value of cross_scenario
def get_primary_secondary_client( | ||
aws_client_factory, | ||
secondary_aws_client_factory, | ||
region_name, | ||
secondary_region_name, | ||
account_id, | ||
secondary_account_id, | ||
): |
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.
as this now part of the global fixtures, it would be good to add a bit of documentation about what this does and the possible values
if custom_aws_client: | ||
sqs_client = custom_aws_client.sqs | ||
else: | ||
sqs_client = aws_client.sqs |
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.
there's a problem with this fixture: it doesn't clean up properly when passing a custom client, because you'll try deleting the queue with the regular aws_client.sqs
instead of the custom client you passed.
You'd need to store the client to the queue_urls
list as well:
queue_urls.append((sqs_client, queue_url))
and then
for sqs_client, queue_url in queue_urls:
try:
sqs_client.delete_queue(QueueUrl=queue_url)
except Exception as e:
LOG.debug("error cleaning up queue %s: %s", queue_url, e)
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 19m 53s ⏱️ Results for commit 9667e96. ♻️ This comment has been updated with latest results. |
03813fc
to
34f9905
Compare
34f9905
to
b56cebd
Compare
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, thanks for addressing the comments! 🚀
Motivation
This PR moves several fixtures from eventbridge conftest to the globally defined fixtures to make them available in pro.
Changes
create_role_event_bus_source_to_bus_target
get_primary_secondary_client
sqs_as_events_target
required if sqs is in separate region / accountsqs_as_events_target