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

refactor: Auth refactor #3

Merged
merged 8 commits into from
Apr 11, 2023
Merged

refactor: Auth refactor #3

merged 8 commits into from
Apr 11, 2023

Conversation

pnadolny13
Copy link
Contributor

@pnadolny13 pnadolny13 commented Apr 11, 2023

This is a first iteration at making the auth and aws base classes more portable and well tested, potentially allowing them to be moved to the SDK or at least directly copied to other taps when needed because this is the second time I've built this:

  • Pulls the auth logic out into a standalone class
  • Pulls the base AWS interface with auth and caching client/resource out to base class that gets inherited by the dynmodb specific class in this tap
  • Lots more tests around the auth steps

Opinionated decisions I made (can be changed):

  • I want to be more explicit about auth parameters. There are lots of taps that accept AWS creds in different ways, some explicit some implicit from env vars or installed files. I'd prefer to to make these all explicit so in this case ENV vars arent used unless a use_aws_env_vars config is set, avoiding confusing behavior where a mix of explicit and implicit inputs are provided. I've been in the case where boto finds local credentials that I'm not intending to use and it causes weird and potentially bad side affects.
  • The order is:
    • access key/secret/region
    • access key/secret/region + session token
    • profile name
    • raise error for now but would like to accept a flag to allow use_implicit_credentials where we dont verify anything and just let boto do its thing
  • Like previously mentioned if use_aws_env_vars is set then credentials are pulled from the env and not from the config keys. My opinion is that we should force one or the other and not a mix.
  • If aws_assume_role_arn is provided then the session created gets passed to the assume role method that creates a new one with the assumed role

Misc Stuff:

  • refresh readme
  • refresh tap config options
  • refresh meltano.yml sample
  • remove TODOs

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.

1 participant