Skip to content

Conversation

@whummer
Copy link
Member

@whummer whummer commented May 18, 2023

This PR adds minor enhancements to have proper handling of the AWS_ENDPOINT_URL configuration. Previously we were only using it for the get_service_endpoint(..) function, but not for get_service_endpoints(..). This was leading to issues when using the

Summary of changes:

  • move logic around AWS_ENDPOINT_URL into get_service_endpoints(..)
  • call get_service_endpoints(..) from get_service_endpoint(..), and select the corresponding service entry from the dict. There is a tiny performance penalty in this approach, which seems acceptable given that it allows us to keep the endpoint construction logic in a central place. (this now also works transitively for get_service_ports(..))
  • update docs to cover $AWS_ENDPOINT_URL configuration

Side note: Overall, this library (esp. the host/port handling bit) should soon become obsolete (there's already a draft PR to remove it from LocalStack core, and we should eliminate it from awslocal soon as well).

@whummer whummer requested a review from simonrw May 18, 2023 10:29
@simonrw
Copy link
Contributor

simonrw commented May 18, 2023

There are no tests to cover this functionality - does it make sense to add tests/a test to check the behaviour of the environments variables (AWS_ENDPOINT_URL, LOCALSTACK_HOST, USE_SSL) to make sure they behave correctly?

@simonrw
Copy link
Contributor

simonrw commented May 18, 2023

There are no tests to cover this functionality - does it make sense to add tests/a test to check the behaviour of the environments variables (AWS_ENDPOINT_URL, LOCALSTACK_HOST, USE_SSL) to make sure they behave correctly?

nvm it's not worth it since as you say

Overall, this library (esp. the host/port handling bit) should soon become obsolete (there's already a draft PR to remove it from LocalStack core, and we should eliminate it from awslocal soon as well).

Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

Works as expected, LGTM! 🎉

@whummer whummer merged commit 0808483 into master May 18, 2023
@whummer whummer deleted the endpoint-url branch May 18, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants