Skip to content
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

Make store lookups for Events rule jobs request context aware #8469

Merged
merged 2 commits into from Jun 14, 2023

Conversation

whummer
Copy link
Member

@whummer whummer commented Jun 9, 2023

Make store lookups for Events rule jobs request context aware. This issue surfaced when restoring rule jobs from persistence/pods (companion PR following soon).

At its core, the PR replaces aws_stack.connect_to_service(..) calls with in-memory lookups, and adds EventsStore as a parameter for the get_scheduled_rule_func/put_rule_job_scheduler util methods, to ensure we're targeting the right account/region.

/cc @giograno

Summary of changes:

  • pass request context to get_store(..), to extract account/region directly from the context object
  • add EventsStore as a parameter for the get_scheduled_rule_func/put_rule_job_scheduler util methods
  • add get_event_bus_name(..) util function that extracts the bus name from a name/ARN, or returns the default event bus as a fallback

@whummer whummer added the semver: patch Non-breaking changes which can be included in patch releases label Jun 9, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 32m 38s ⏱️
2 088 tests 1 805 ✔️ 283 💤 0
2 089 runs  1 805 ✔️ 284 💤 0

Results for commit 0b1ee21.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

Coverage Status

coverage: 82.756% (+0.04%) from 82.721% when pulling 0b1ee21 on events-store-ctx into 1d35c60 on master.

@embano1
Copy link

embano1 commented Jun 9, 2023

@whummer while searching for a solution for my issue (potential bug), I found this PR which is in the same code path. It does not fix my bug, but I wanted to reach out to you since you're familiar with that code base and was wondering whether that's an easy fix you could amend to this PR? #8472

Copy link
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 👍

@whummer
Copy link
Member Author

whummer commented Jun 14, 2023

Hi @embano1 . The issue you've reported could potentially be resolved with this change: https://github.com/localstack/localstack/pull/8469/files#diff-e93fb0ca12285e3a1b71692d6dcae8cf7333857a6019bbe5af6115615541d088R519-R521 The latest Docker image will be available ~1h after merging this PR, can you then please pull it and give it a try? Thanks!

@whummer whummer merged commit 7e00203 into master Jun 14, 2023
28 checks passed
@whummer whummer deleted the events-store-ctx branch June 14, 2023 09:17
@embano1
Copy link

embano1 commented Jun 14, 2023

@whummer you're awesome, will try ASAP and report back in the issue!

@embano1
Copy link

embano1 commented Jun 14, 2023

@whummer been 3h since you merged and Docker Hub still shows the old one from 7h ago
Is there something broken in CI?

image

@whummer
Copy link
Member Author

whummer commented Jun 14, 2023

@embano1 , looks like indeed we had some flakiness in our CI pipeline, re-triggered the build one more time, should hopefully update the latest image soon.

You can follow the progress here: https://app.circleci.com/pipelines/github/localstack/localstack?branch=master Thanks

@embano1
Copy link

embano1 commented Jun 14, 2023

@whummer great, thx a bunch!

@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants