Skip to content

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Mar 21, 2023

Motivation

Currently, the new AWS provider allows us to set internal request parameters with an underscore in front of the parameter name in Pascal Case, like this:

connect_to().lambda.list_functions(_ServicePrincipal="apigateway")

Due to the typed nature of the new clients, this will lead to a typing error, as these parameters are not known by the boto stubs.

Changes

Introduces another method to all returned clients, named request_metadata. You can use this method to get a "version" of your client where these parameters are automatically set on all service calls.

Example:

connect_to().lambda.request_metadata(service_principal="apigatway").list_functions()

This will be typed correctly, which removes all the yellow warnings of your IDE.

Drawback

The main drawback is the highly weird way this is done in. It introduces a new "proxy" object, which will be typed as the boto3 client. On any get attribute operation for a callable (like a client.function), it will return a partial function with the above mentioned parameters pre-filled, so you do not have to provide them.

This is quite unorthodox, but works fine without too much of a performance penalty.

@dfangl dfangl requested a review from thrau as a code owner March 21, 2023 16:46
@dfangl dfangl temporarily deployed to localstack-ext-tests March 21, 2023 16:46 — with GitHub Actions Inactive
if service_principal:
params["_ServicePrincipal"] = service_principal
self._params = params
return self
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I think it would be better to make this immutable and always return a new object

@dfangl dfangl temporarily deployed to localstack-ext-tests March 21, 2023 17:14 — with GitHub Actions Inactive
@dfangl dfangl temporarily deployed to localstack-ext-tests March 21, 2023 17:30 — with GitHub Actions Inactive
@dfangl dfangl temporarily deployed to localstack-ext-tests March 21, 2023 17:33 — with GitHub Actions Inactive
@thrau thrau removed their request for review March 21, 2023 17:33
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! 🚀
Added a minor doc suggestion.

Really itching to rewrite some things with the new client now!

from typing import TYPE_CHECKING, Union

if TYPE_CHECKING:
from mypy_boto3_acm import ACMClient
Copy link
Member

Choose a reason for hiding this comment

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

Love that these are now separated into their own file. Will keep the churn here much better isolated 👍

Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
@dfangl dfangl temporarily deployed to localstack-ext-tests March 21, 2023 18:58 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Mar 21, 2023

LocalStack integration with Pro

       3 files  ±  0         3 suites  ±0   1h 34m 19s ⏱️ - 2m 9s
1 838 tests +  6  1 449 ✔️ +7  389 💤  - 1  0 ±0 
2 574 runs  +14  1 816 ✔️ +7  758 💤 +7  0 ±0 

Results for commit 16072e3. ± Comparison against base commit 814bbe8.

♻️ This comment has been updated with latest results.

@dfangl dfangl temporarily deployed to localstack-ext-tests March 22, 2023 09:32 — with GitHub Actions Inactive
@dfangl dfangl temporarily deployed to localstack-ext-tests March 22, 2023 09:41 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

Coverage: 84.848% (-0.06%) from 84.909% when pulling 16072e3 on aws-client-additions into 814bbe8 on master.

Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

LGTM!

@dfangl dfangl merged commit 85a00cb into master Mar 23, 2023
@dfangl dfangl deleted the aws-client-additions branch March 23, 2023 08:24
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.

4 participants