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

Ensure properly configured client for fixtures and testutils#8892

Merged
viren-nadkarni merged 10 commits into
masterfrom
client-account-id-context
Aug 23, 2023
Merged

Ensure properly configured client for fixtures and testutils#8892
viren-nadkarni merged 10 commits into
masterfrom
client-account-id-context

Conversation

@viren-nadkarni
Copy link
Copy Markdown
Member

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

This PR make a few fixes to usage of the internal clients to make use of proper account ID/region from request context.

This is part of the continuing effort to improve cross account namespacing in LocalStack.

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

github-actions Bot commented Aug 11, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 17m 29s ⏱️
2 110 tests 1 661 ✔️ 449 💤 0
2 111 runs  1 661 ✔️ 450 💤 0

Results for commit db728ac.

♻️ This comment has been updated with latest results.

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 15, 2023

Coverage Status

coverage: 80.719% (-0.004%) from 80.723% when pulling db728ac on client-account-id-context into 2eddd22 on master.

Copy link
Copy Markdown
Member

@giograno giograno left a comment

Choose a reason for hiding this comment

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

Looks good 👍


def get_rest_api_paths(rest_api_id, region_name=None):
apigateway = connect_to(region_name=region_name).apigateway
def get_rest_api_paths(account_id: str, region_name: str, rest_api_id):
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.

nit/question: regione_name was optional and now it's not. Do we always expect it or it can be None?
rest_api_id might get a str type hint.

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.

It can be None from the perspective of the internal client library (connect_to). However, we want to be as explicity as possible with account IDs and regions to keep expectable behaviour with cross-account usage :)

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.

Type hint added in 9ef697d

@viren-nadkarni viren-nadkarni merged commit b3b7ce8 into master Aug 23, 2023
@viren-nadkarni viren-nadkarni deleted the client-account-id-context branch August 23, 2023 08:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

3 participants