-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: boto3 connector #1655
base: main
Are you sure you want to change the base?
feat: boto3 connector #1655
Conversation
for more information, see https://pre-commit.ci
I also see that theres poetry.lock conflicts. Any recommendations on the best way youv'e found to merge those? |
Rebase, ensure you have the latest pyproject toml, run poetry lock --no-update, the commit the updated lock file. Or just use the upstream lock file. EDIT: just sharing what I do with poetry |
for more information, see https://pre-commit.ci
@edgarrmondragon @kgpayne any thoughts on this PR? |
if config.get("use_aws_env_vars"): | ||
self.aws_access_key_id = os.environ.get("AWS_ACCESS_KEY_ID") | ||
self.aws_secret_access_key = os.environ.get("AWS_SECRET_ACCESS_KEY") | ||
self.aws_session_token = os.environ.get("AWS_SESSION_TOKEN") | ||
self.aws_profile = os.environ.get("AWS_PROFILE") | ||
self.aws_default_region = os.environ.get("AWS_DEFAULT_REGION") | ||
else: | ||
self.aws_access_key_id = config.get("aws_access_key_id") | ||
self.aws_secret_access_key = config.get("aws_secret_access_key") | ||
self.aws_session_token = config.get("aws_session_token") | ||
self.aws_profile = config.get("aws_profile") | ||
self.aws_default_region = config.get("aws_default_region") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for users to want to use a mix of env vars and JSON config? For example, set AWS_ACCESS_KEY_ID
and AWS_SECRET_ACCESS_KEY
in their environment but switch regions via the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarrmondragon potentially, I dont know all the common patterns people use but my vision for this was to try to make it more explicit where AWS credentials are coming from. I find that having so many variations of where credentials come from (json, profiles, env vars, machine inheritances) and potentially mixing them makes it confusing and sometimes leads to a tap using credentials you weren't intending to use. What do you think about that? Am I exaggerating the challenge a bit?
In tap-dynamodb I was raising an error if credentials weren't provided but some users need to use implicit credentials MeltanoLabs/tap-dynamodb#14. This has me thinking about adding a config option like use_implicit_credentials=True
, meaning the user needs to explicitly tell the tap that it should use a naked call like boto3.Session()
with no args/kwargs to defer to the boto hierarchy of searching for credentials on the machine.
I'm also almost in favor of eliminating the use_aws_env_vars
option and requiring that all users configure them via their meltano.yml. If they want to access env vars they should use templating in their meltano.yml explicitly.
Any thoughts?
@pnadolny13 I'm still iterating on the connector interface but the guide in in https://meltano-sdk--1649.org.readthedocs.build/en/1649/guides/custom-connector.html gives an idea of where I'd want it to go. Currently only the |
@edgarrmondragon that makes sense. Its a little awkward with boto3 since it already handles opening and closing the session for you so I'd be adding a context manager for basically no purpose other than to fit the connector abstraction. I'm not quite sure how this works but could I override |
@edgarrmondragon any new thoughts on this PR? I know you said you were still iterating on the connector interface. Is this something that we'd want to merge sometime soon? If so, I can clean it up to use the |
This has been marked as stale because it is unassigned, and has not had recent activity. It will be closed after 21 days if no further activity occurs. If this should never go stale, please add the |
Closes #1651
@edgarrmondragon I see in #1649 that you're working on a base connector implementation. I'm happy to refactor this to match that structure once its ready.
📚 Documentation preview 📚: https://meltano-sdk--1655.org.readthedocs.build/en/1655/