-
Notifications
You must be signed in to change notification settings - Fork 218
feat: add Microsoft Fabric and SQL Server support to elementary CLI #2140
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
Changes from 19 commits
d958b2a
ac95204
6a737e8
b54a7a1
62a36e6
ba52254
cfc9b68
7b51cd7
1739662
65d032e
f51dfc9
dff9bc3
699bee3
1f23903
a86d990
613d8ed
d566f90
40c8044
39a3c8d
5461453
6989836
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -100,12 +100,14 @@ jobs: | |||||
| trino, | ||||||
| dremio, | ||||||
| spark, | ||||||
| fabric, | ||||||
| sqlserver, | ||||||
| ] | ||||||
| uses: ./.github/workflows/test-warehouse.yml | ||||||
| with: | ||||||
| warehouse-type: ${{ matrix.warehouse-type }} | ||||||
| elementary-ref: ${{ inputs.elementary-ref || ((github.event_name == 'pull_request_target' || github.event_name == 'pull_request') && github.event.pull_request.head.sha) || '' }} | ||||||
| dbt-data-reliability-ref: ${{ inputs.dbt-data-reliability-ref }} | ||||||
| dbt-data-reliability-ref: ${{ inputs.dbt-data-reliability-ref || 'devin/ELE-5282-1772640713' }} | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 CI workflow hardcodes a development branch for dbt-data-reliability-ref The
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intentional and already documented in the PR description — this PR depends on the |
||||||
| dbt-version: ${{ matrix.dbt-version }} | ||||||
| generate-data: ${{ inputs.generate-data || false }} | ||||||
| secrets: inherit | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ on: | |
| - duckdb | ||
| - trino | ||
| - dremio | ||
| - fabric | ||
| - sqlserver | ||
| elementary-ref: | ||
| type: string | ||
| required: false | ||
|
|
@@ -159,6 +161,13 @@ jobs: | |
| run: | | ||
| docker compose up -d --build --wait spark-thrift | ||
|
|
||
| - name: Start SQL Server | ||
| if: inputs.warehouse-type == 'sqlserver' | ||
| working-directory: ${{ env.E2E_DBT_PROJECT_DIR }} | ||
| run: | | ||
| docker compose up -d sqlserver | ||
| timeout 120 bash -c 'until docker exec sqlserver /opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P "${MSSQL_SA_PASSWORD:-Elementary123!}" -C -Q "SELECT 1" -b 2>/dev/null; do sleep 5; done' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a healthcheck instead in the docker compose, and then wait for the docker to be healthy?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — the docker-compose already had a healthcheck defined, so I've updated the CI step to use |
||
|
|
||
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
|
|
@@ -168,6 +177,14 @@ jobs: | |
| if: inputs.warehouse-type == 'spark' | ||
| run: sudo apt-get install -y python3-dev libsasl2-dev gcc | ||
|
|
||
| - name: Install ODBC driver for SQL Server | ||
| if: inputs.warehouse-type == 'fabric' || inputs.warehouse-type == 'sqlserver' | ||
| run: | | ||
| curl -fsSL https://packages.microsoft.com/keys/microsoft.asc | sudo tee /etc/apt/trusted.gpg.d/microsoft.asc > /dev/null | ||
| curl -fsSL https://packages.microsoft.com/config/ubuntu/$(lsb_release -rs)/prod.list | sudo tee /etc/apt/sources.list.d/mssql-release.list > /dev/null | ||
| sudo apt-get update | ||
| sudo ACCEPT_EULA=Y apt-get install -y msodbcsql18 unixodbc-dev | ||
|
haritamar marked this conversation as resolved.
|
||
|
|
||
| - name: Install dbt | ||
| run: > | ||
| pip install | ||
|
|
@@ -188,7 +205,7 @@ jobs: | |
| # This enables caching the seeded database state between runs. | ||
| IS_DOCKER=false | ||
| case "${{ inputs.warehouse-type }}" in | ||
| postgres|clickhouse|trino|dremio|duckdb|spark) IS_DOCKER=true ;; | ||
| postgres|clickhouse|trino|dremio|duckdb|spark|sqlserver) IS_DOCKER=true ;; | ||
| esac | ||
|
|
||
| if [ "$IS_DOCKER" = "true" ]; then | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| {%- macro edr_quote_identifier(identifier) -%} | ||
| {%- if elementary.is_tsql() -%} | ||
| [{{ identifier }}] | ||
| {%- else -%} | ||
| {{ identifier }} | ||
| {%- endif -%} | ||
| {%- endmacro -%} |
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.
This changes the default
dbt-data-reliability-reffor all warehouse CI runs (not just fabric/sqlserver). Once ELE-5282 is merged, this default needs to be reverted to empty — otherwise all CI runs will pin to a stale branch.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.
Acknowledged — the default will need to be reverted to empty once ELE-5282 is merged. I'll leave a
TODOnote or we can address it when that branch lands.