Remove last uses of get_aws_account_id() and aws_stack.get_region() #9641
Conversation
viren-nadkarni
added
the
semver: patch
| account_id = context.account_id or DEFAULT_AWS_ACCOUNT_ID | ||
| region_name = context.region_name or AWS_REGION_US_EAST_1 |
There was a problem hiding this comment.
This mimics the existing behaviour. At the time of executing this function, the account ID and region enricher handlers are not executed in the handler chain:
localstack/localstack/aws/app.py
Line 38 in f7eff1f
These enricher handlers are responsible for setting the values in the thread context storage. Thus, the get_aws_account_id() and aws_stack.get_region() return the fallback values.
There was a problem hiding this comment.
Thanks for the explanation! I hope this doesn't fallback often, I guess it falls back before actually failing but at least we have values. This makes sense and will be revisited in an upcoming APIGW refactor! thanks a lot
viren-nadkarni
force-pushed
the
remove-context-utils
branch
from
f7eff1f to
58711ac
Compare
| ): | ||
| """Fix the account ID in the ARNs returned in the given Flask response or string""" | ||
| existing = existing or ["123456789", "1234567890", "123456789012", get_aws_account_id()] | ||
| existing = existing or ["123456789", "1234567890", DEFAULT_ACCOUNT_ID] |
There was a problem hiding this comment.
Remove hardcoded Moto default account ID
viren-nadkarni
requested review from
bentsku,
calvernaz,
dfangl,
dominikschubert,
joe4dev,
pinzon,
sannya-singal,
silv-io,
simonrw and
steffyP
as code owners
viren-nadkarni
removed request for
calvernaz,
joe4dev,
pinzon,
sannya-singal,
silv-io and
simonrw
bentsku
left a comment
bentsku
left a comment
There was a problem hiding this comment.
LGTM! This is great again, nice cleaning up! 🧹 that is quite the effort that lead to this final removal. Awesome!
Also, nice spotting the bug in the APIGW SQS integration. It might need a follow up on our side to make the URL construction depend on the SQS URL strategy.
| account_id = context.account_id or DEFAULT_AWS_ACCOUNT_ID | ||
| region_name = context.region_name or AWS_REGION_US_EAST_1 |
There was a problem hiding this comment.
Thanks for the explanation! I hope this doesn't fallback often, I guess it falls back before actually failing but at least we have values. This makes sense and will be revisited in an upcoming APIGW refactor! thanks a lot
| url = urljoin(config.internal_service_url(), f"{get_aws_account_id()}/{queue}") | ||
| url = urljoin(config.internal_service_url(), f"{account_id}/{queue}") |
There was a problem hiding this comment.
Oh, damn, this was actually a bad bug 😬 shouldn't this URL depends on the SQS URL strategy actually?
There was a problem hiding this comment.
Not necessarily, URL strategy controls how queues will be formatted in outputs. SQS will still recognise other queue URL formats if presented. Moreover the use of config.internal_service_url() prevents us from using the standard and domain strategies.
But you're right in another way, the region is currently omitted in the queue URL. I've added it with 71b862a. Great catch, thanks!
viren-nadkarni
force-pushed
the
remove-context-utils
branch
from
1f06f0e to
2ab9534
Compare
viren-nadkarni
force-pushed
the
remove-context-utils
branch
from
2ab9534 to
b26f3f6
Compare
Motivation
This PR removes last notable remaining uses of
get_aws_account_id()andaws_stack.get_region().These functions will be marked as deprecated or removed altogether in a subsequent PR.