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

[#173] Boto3 client kwargs passthrough #176

Merged
merged 1 commit into from
Nov 26, 2022

Conversation

nk9
Copy link

@nk9 nk9 commented Nov 21, 2022

I have not tested this! I don't have an SRP setup.

Please test it @Amejonah1200 and let me know if this will do what you need.

@Amejonah1200
Copy link

It looks functional, but this solution doesn't allow IDEs to check for parameters (also mo auto-complete) and it has a sideeffect of modifying input dictionary.

@nk9
Copy link
Author

nk9 commented Nov 23, 2022

This is true. Here is the list of parameters accepted by the function. And these are the ones missing right now:

  • api_version
  • aws_session_token
  • use_ssl
  • verify
  • endpoint_url

So this PR would allow all 5 of those to be set as needed, and with a minimum of code changes, but at the expense of missing IDE sugar. I could also do it with **kwargs in the signature, but that won't improve the experience for IDE users.

Thoughts @pvizeli?

@Amejonah1200
Copy link

You can also just make them return default values, this would also not require code changes (add also type hints).

@nk9
Copy link
Author

nk9 commented Nov 23, 2022

I understand what you're suggesting, but I'm saying it's a larger change because then we have to declare all the variables twice, pass them into the utils function, and then conditionally pack them into the dictionary to be unpacked again into the client function call. I'm looking for guidance from the maintainer as to what approach he'd prefer.

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

I'm fine with that approach, pycognito is an wrapper around boto3 cognito. We doing things like this on other place too. Agree, maybe not that optimal, but if someone will add typehints, that work with TypedDicts. Fine for me to ditch support for python < 3.8

@pvizeli pvizeli merged commit 073caa4 into NabuCasa:master Nov 26, 2022
@nk9
Copy link
Author

nk9 commented Nov 27, 2022

Great, thanks. BTW, I've run the code now and verified that it's succesfully passing values through.

@nk9 nk9 deleted the nk-srm-passthrough branch November 27, 2022 12:55
@Amejonah1200
Copy link

Thanks everyone!

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.

None yet

3 participants