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

vertica driver support #393

Merged
merged 2 commits into from
Dec 5, 2022
Merged

vertica driver support #393

merged 2 commits into from
Dec 5, 2022

Conversation

bobpace
Copy link
Contributor

@bobpace bobpace commented Aug 5, 2022

  • Add VerticaDialect
  • Add vertica-sql-go dependency
  • Add !no_vertica build tag in driver_vertica.go
  • Updated README
  • Updated CLI help message
  • Test coverage

I've been using these changes successfully already with a custom build of goose to manage vertica migrations for a project. I'm willing to include test coverage similar to what has been done for other drivers if there is interest in accepting this PR. Let me know if there is any other feedback you have for me that I may have missed. Thanks!

@mfridman
Copy link
Collaborator

Seems reasonable, I have not personally used vertica, but judging by the queries it seems pretty straightforward.

I presume we can use a docker container, such as:

https://hub.docker.com/r/vertica/vertica-ce

@bobpace
Copy link
Contributor Author

bobpace commented Aug 15, 2022

I was able to get the container to work with a few modifications to reduce the startup time:

  • setting VMART_ETL_SCRIPT env variable to empty string to prevent loading VMART schema
  • mounting empty folder over /opt/vertica/packages to prevent additional package installation steps

Only one instance of Vertica can be running on a host at any time so I put all assertions into one unit test.

@thackerc
Copy link

@mfridman I'd love to see this merged. Any concerns on your end?

@mfridman
Copy link
Collaborator

No concerns with the PR itself, more concern about the ongoing maintenance.

I'm not familiar with vertica DB, so if users start filing issues/ask questions there is an additional maintenance cost to the current and future maintainers.

I'm starting to think there might be "official" and "third-party" goose support for the various databases and drivers.

@bobpace
Copy link
Contributor Author

bobpace commented Sep 19, 2022

What do you think about instead of this PR just exporting the functions on the SQLDialect interface so it can be implemented outside the goose package and offering something like:

func SetDialectThirdParty(d SQLDialect) {
	dialect = d
}

@tmm1
Copy link

tmm1 commented Nov 8, 2022

What do you think about instead of this PR just exporting the functions on the SQLDialect interface so it can be implemented outside the goose package and offering something like:

Interesting idea, but that would mean you could only use it from the golang API right, and the cli would not work?

@mfridman
Copy link
Collaborator

Hey @bobpace, if you're up for it, can you rebase (or merge) main, overall looks good to me and we can merge this in.

@bobpace
Copy link
Contributor Author

bobpace commented Dec 2, 2022

That sounds great, thanks!

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