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

Break out host/port/username configurations #30

Closed
visch opened this issue Feb 1, 2023 · 2 comments · Fixed by #121
Closed

Break out host/port/username configurations #30

visch opened this issue Feb 1, 2023 · 2 comments · Fixed by #121

Comments

@visch
Copy link
Member

visch commented Feb 1, 2023

No description provided.

@qbatten
Copy link
Contributor

qbatten commented Feb 7, 2023

Dupe of #5, I think. I can push a PR for this if I get a ruling on whether we should

  • allow either sqlalchemy_url or host/port/etc
  • switch over only to host/port/etc and drop sqlalchemy_url altogether

I don't care either way. Here's a repost of my comment on 5:

Per @/aaronsteers comment here, should we drop the sqlalchemy_url option altogether? @/visch what do you think about that? Seems like the pro/con of allowing users to pass sqlalchemy_url is something like:

Pro: Flexible for users
Con: Leaves the tap with less control; could cause things to break if the tap upgrades driver and a user isn't aware of that

Sounds like either way we'll exclude the dialect+driver option in the broken-out configuration.

@aaronsteers
Copy link

aaronsteers commented Apr 3, 2023

Hi, @qbatten. We ran into this elsewhere also. While the connector is still young, this is the right time to make a breaking change and remove support for arbitrary sqlalchemy_url inputs. Overrides or additions to the base string could optionally be exposed as "extra_sqlalchemy_args" config option.

Driver-specific implementation choices should be abstracted away from the users, so that if we change the driver or certain implementation details without breaking (future) users.

visch added a commit that referenced this issue Jun 23, 2023
…alues (#121)

Closes #30, #5

- [x] Need to implement SSL as a configuration option as well (Going to
do separately see #117
)

---------

Co-authored-by: Edgar R. M <[email protected]>
Co-authored-by: Pat Nadolny <[email protected]>
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 a pull request may close this issue.

3 participants