-
Notifications
You must be signed in to change notification settings - Fork 130
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
OLAP Connectors: Test the connection before exiting the "Add Data" modal #6892
base: main
Are you sure you want to change the base?
Conversation
|
||
test.describe("ClickHouse connector", () => { | ||
test.use({ project: "Blank" }); | ||
|
||
const clickhouse = new ClickHouseTestContainer(); | ||
|
||
test.beforeAll(async () => { |
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 be good to move this to a static method on ClickHouseTestContainer that also returns the instance.
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.
I can't quite tell exactly what you have in mind? Regarding another layer of abstraction– that might be valuable, but I'd prefer not to add extra abstractions here before we know how our usage of this class will evolve. For example, I could especially see the seed
method being parameterized based on which dataset you want to use for a given test suite.
This PR ensures that a connection to the OLAP database is successfully established before finalizing the creation of a
connector.yaml
file and updating the.env
file. Previously, submitting the ClickHouse, Druid, or Pinot connector forms would create these files regardless of the validity of the credentials.Implementation details:
connector.yaml
file..env
file with the provided credentials.OLAPListTables
API to verify the connection..env
file, and display an error message.Once #6671 lands, we can potentially switch the implementation to use a
tmp
directory and, in particular, atmp/.env
file to test credentials.Checklist: