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
Automate releasing embedding SDK #42516
Conversation
06b5cbd
to
a7c1de2
Compare
a7c1de2
to
c038d2e
Compare
7997bc5
to
c038d2e
Compare
Because OSS is never needed
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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.
Left some comments that I hope you'll find usefull.
Just to repeat once again, I think we should start with a separate uberjar-sdk.yml workflow until we learn how are we going to use this.
Great work overall! 🥇
commit: | ||
description: 'Optional full-length commit SHA-1 hash' |
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.
We might want to make commit
required in the future.
This really depends on how we end up using this workflow, or rather - what are we going to "tie" the SDK to.
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.
Fixed in 0639cfb
concurrency: | ||
# We want to ensure only one job is running at a time because | ||
# there could be a conflict when updating the readme file. | ||
group: ${{ github.workflow }} | ||
cancel-in-progress: true |
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.
👌
run: | | ||
git push origin ${{ env.tag }} | ||
- if: failure() | ||
run: echo "Make sure the tag '${{ env.tag }}' doesn't exist." |
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 would probably expand this message a bit.
A failure can happen for many reasons. Just one of them can be that the tag exists.
Btw, this is the reason why I prefer what @iethree did with the release process where he is using the official GitHub's "OctoKit" REST endpoints. The response might contain the reason why something failed.
runs-on: ubuntu-22.04 | ||
timeout-minutes: 20 | ||
steps: | ||
- name: Check out the code using the provided commit, or HEAD |
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.
Please update the description to reflect the proper ref
used here.
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.
Fixed in 0639cfb
name: metabase-${{ github.sha }}-sdk | ||
path: ./resources/embedding-sdk | ||
|
||
build-jar: |
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.
If this has the chance to be only a temporary solution, I wouldn't alter the existing uberjar.yml
workflow (it's heavy already).
WDYT about creating a temporary uberjar-sdk.yml
instead?
The benefit is that you can create it either as a workflow or as a composite action even.
I feel 90% confident that we should not modify the existing uberjar workflow for now.
runs-on: ubuntu-22.04 | ||
timeout-minutes: 20 | ||
steps: | ||
- name: Check out the code using the provided commit, or HEAD |
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.
Please update the description, or omit it.
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.
fixed in 0639cfb
- name: Check out the code using the provided commit, or HEAD | ||
uses: actions/checkout@v4 | ||
with: | ||
ref: master |
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 would be useful to have as an actual comment in the code.
.github/workflows/uberjar.yml
Outdated
@@ -26,15 +36,15 @@ jobs: | |||
timeout-minutes: 40 | |||
strategy: | |||
matrix: | |||
edition: [ee, oss] | |||
edition: ${{ fromJSON(inputs.only_ee && '["ee"]' || '["ee", "oss"]') }} |
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 think we shouldn't modify the existing workflow.
But if we do end up doing that, the alternative to parameterizing the matrix is to have a conditional step execution. You can provide something like if workflow_call AND edition is EE
, then execute this step.
This comes with tradeoffs, for sure.
If you have to do this for every single step out there, then it makes sense to parameterize the matrix, for sure.
Please check include
and exclude
.github/workflows/uberjar.yml
Outdated
env: | ||
MB_EDITION: ${{ matrix.edition }} | ||
INTERACTIVE: false | ||
steps: | ||
- name: Check out the code | ||
uses: actions/checkout@v4 | ||
with: | ||
ref: ${{ github.event.inputs.commit }} | ||
ref: ${{ inputs.commit }} |
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.
But the commit
that comes from SDK is optional.
Which means that it's going to default to the HEAD
.
So what happens in SDK builds from commit abc
on master
, and by the time this workflow kicks in someone pushed another commit. In that case the uberjar workflow will build from commit def
.
This is not hard to imagine given the frequency we push to master
(especially just prior to the release).
78650e3
to
5df46dd
Compare
env: | ||
BUCKET: ${{ vars.AWS_S3_DOWNLOADS_BUCKET }} | ||
BUCKET_PATH: sdk/v${{ inputs.sdk_version }}/metabase.jar | ||
OUTPUT_FILE: ./target/uberjar/metabase.jar |
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 find the name "output file" a bit ambiguous in this context as it's used as the first parameter of cp, can we just inline it in the cp command as it seems it's only used once?
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 shamelessly copied that from
metabase/.github/actions/upload-test-results/action.yml
Lines 40 to 41 in 025d62c
BUCKET: ${{ inputs.bucket }} | |
OUTPUT_FILE: ${{ inputs.output-name }} |
The reason I don't inline all of theme since it makes it hard to read since the line would be superlong. Would renaming this env instead help? e.g. INPUT_FILE
or just FILE
since we're copying files also I think we don't need a bracket just $FILE
should work
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.
Fixed in 87b22fc
app-id: ${{ secrets.METABASE_BOT_APP_ID }} | ||
private-key: ${{ secrets.METABASE_BOT_APP_PRIVATE_KEY }} | ||
|
||
- name: Create a PR updating readme + published version |
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.
Just to be sure, this PR that gets created will update the versions, but the package with the specified version will already be published right?
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.
yeah, but to be specific the package would be published in the last step within this job. The PR would have been created at this step and we will have to manually merge the PR.
Realistically, by the time we look at the PR, the SDK package would already likely be published.
@@ -0,0 +1,149 @@ | |||
name: Build + Docker Uberjar for SDK |
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 is copied over from uberjar.yml
and
- strip out unused parts e.g. steps that only run on master
- all triggers outside
workflow_call
- Simplify the artifact name to
metabase-ee-uberjar
- accepts a new input
image_name
previously the image name is determined from a branch name that triggers the workflow, but we want to use the tag name (which created within the workflow) instead.
.github/workflows/uberjar-sdk.yml
Outdated
- name: Retag and push images if dev branch | ||
if: ${{ !(startsWith(github.ref_name,'master') || startsWith(github.ref_name,'backport')) && matrix.edition == 'ee' }} | ||
run: docker tag localhost:5000/metabase-dev:${{ env.image_name }}-ee ${{ github.repository_owner }}/metabase-dev:${{ env.image_name }} && docker push ${{ github.repository_owner }}/metabase-dev:${{ env.image_name }} |
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.
So, if you build from master
, this will not push to DockerHub.
Is it the intended behavior?
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.
good point. I need to fix this. Not intended by any mean. This is just a stripdown version of uberjar.yml
|
||
- name: Create a new git tag | ||
run: | | ||
git tag ${{ env.tag }} |
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.
Nit:
Do we intentionally provide only a lightweight tag here?
cc @WiNloSt
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.
We don't have any information we want to include in the annotated tag other than repeating the version in the tag name itself, so it should be safe to use a lightweight tag here.
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.
It's always tricky to review GH actions workflows because it's so easy to overlook something. But you're already testing the workflows by running then successfully ✅
Overall, this looks good to me.
Thanks for addressing the comments.
P.S. Depending on what the future for SDK holds, I think it makes sense to move a lot of the logic to the release/
folder that @iethree maintains. That would give you the ability to test and validate utils thoroughly.
Closes #42498
Description
This PR adds a workflow that will be triggered by
workflow_dispatch
by providing an optionalcommit
sha.How to verify
0.1.3-test1
as thesdk_version
input to the workflowuberjar.yml
workflow only builds 1 EE jar since we don't need OSS for this.--dry-run
flag). The result is here