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

Add initial partner driver testing github action #9

Merged
merged 73 commits into from
Dec 11, 2024
Merged

Conversation

snoe
Copy link
Collaborator

@snoe snoe commented Dec 2, 2024

Runs metabase test suite against every driver in registry.yml
Can run local docker instances of driver dbs or use credentials in 1password to access hosted ci instances.
New refs key can point from a metabase version, say 53 to a reference on the driver remote repo if needed.

@snoe snoe self-assigned this Dec 2, 2024
@snoe snoe requested a review from iethree December 9, 2024 23:06
uses: actions/setup-java@v2
with:
distribution: temurin
java-version: 21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to test this on a few different versions: at least 11 and 21

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 21 should be sufficient for now, as we get more in the weeds having a mechanism for multiple java versions would make sense.

- name: Setup Node
uses: actions/setup-node@v2
with:
node-version: "22"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of our stuff is still on node v18

Comment on lines +118 to +128
- name: Build jar
run: |
echo "{:deps {metabase/${{ inputs.DRIVER }} {:local/root \"${{ inputs.DRIVER }}\" }}}" > modules/drivers/deps.edn
bin/build-driver.sh ${{ inputs.DRIVER }}
mv resources/modules/${{ inputs.DRIVER }}.metabase-driver.jar ${{ inputs.DRIVER }}.${{ inputs.MB_REF }}.metabase-driver.jar

- name: Upload driver jar
uses: actions/upload-artifact@v4
with:
name: ${{ inputs.DRIVER }}.${{ inputs.MB_REF }}.metabase-driver.jar
path: metabase/${{ inputs.DRIVER }}.${{ inputs.MB_REF }}.metabase-driver.jar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we saving these artifacts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to move towards us building and using the artifacts that we include in cloud rather than relying on partner builds.

Comment on lines 4 to 5
schedule:
- cron: "0 * * * *"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😳 running this every hour on the hour?

on:
schedule:
- cron: "0 * * * *"
push:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe on push to main?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all branches is actually correct, since PRs would affect the runners and should ensure everything is working (even if not passing)

Comment on lines 11 to 14
outputs:
versions: ${{ steps.versions.outputs.value }}
drivers: ${{ steps.drivers.outputs.value }}
steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

little tip: you can set this once for the whole job at the job level, which also makes it easier to read:

env:
  CURRENT_VERSION: ${{ vars.CURRENT_VERSION }}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we should check and make sure this variable is an org-level variable, and not specific to each repo so we don't have to keep them in sync, which we will 100% fail to do at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is org level.

@snoe snoe requested a review from iethree December 10, 2024 19:28
Copy link
Contributor

@iethree iethree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as a foundation to get started!

@snoe snoe merged commit 51cdec9 into main Dec 11, 2024
5 of 23 checks passed
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.

2 participants