Automate releasing embedding SDK #42516

WiNloSt · 2024-05-10T15:40:30Z

Description

This PR adds a workflow that will be triggered by workflow_dispatch by providing an optional commit sha.

How to verify

I tested using 0.1.3-test1 as the sdk_version input to the workflow
The current uberjar.yml workflow only builds 1 EE jar since we don't need OSS for this.
The build passes
The PR to update docs + package.json template. Only contains 3 line changes to 2 files regarding the version number upgrade.
Docker image is updated at https://hub.docker.com/layers/metabase/metabase-dev/embedding-sdk-0.1.3-test1/images/sha256-74c6374bbbaf90bf9a805d40f06b54221465cf537ef977d6b528baffaf838360?context=explore
Jar file is updated at https://downloads.metabase.com/sdk/v0.1.3-test1/metabase.jar
The npm package is published at (TODO: Implement the publish step, now it only runs with --dry-run flag). The result is here

Because OSS is never needed

github-advanced-security · 2024-05-13T16:01:48Z

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.

nemanjaglumac

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! 🥇

nemanjaglumac · 2024-05-16T08:20:37Z

.github/workflows/release-embedding-sdk.yml

+      commit:
+        description: 'Optional full-length commit SHA-1 hash'


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.

Fixed in 0639cfb

nemanjaglumac · 2024-05-16T08:20:50Z

.github/workflows/release-embedding-sdk.yml

+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


nemanjaglumac · 2024-05-16T08:22:33Z

.github/workflows/release-embedding-sdk.yml

+        run: |
+          git push origin ${{ env.tag }}
+      - if: failure()
+        run: echo "Make sure the tag '${{ env.tag }}' doesn't exist."


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.

nemanjaglumac · 2024-05-16T08:23:04Z

.github/workflows/release-embedding-sdk.yml

+    runs-on: ubuntu-22.04
+    timeout-minutes: 20
+    steps:
+      - name: Check out the code using the provided commit, or HEAD


Please update the description to reflect the proper ref used here.

Fixed in 0639cfb

nemanjaglumac · 2024-05-16T08:28:50Z

.github/workflows/release-embedding-sdk.yml

+          name: metabase-${{ github.sha }}-sdk
+          path: ./resources/embedding-sdk
+
+  build-jar:


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.

nemanjaglumac · 2024-05-16T08:30:19Z

.github/workflows/release-embedding-sdk.yml

+    runs-on: ubuntu-22.04
+    timeout-minutes: 20
+    steps:
+      - name: Check out the code using the provided commit, or HEAD


Please update the description, or omit it.

fixed in 0639cfb

nemanjaglumac · 2024-05-16T08:30:36Z

.github/workflows/release-embedding-sdk.yml

+      - name: Check out the code using the provided commit, or HEAD
+        uses: actions/checkout@v4
+        with:
+          ref: master


This would be useful to have as an actual comment in the code.

nemanjaglumac · 2024-05-16T08:35:16Z

.github/workflows/uberjar.yml

@@ -26,15 +36,15 @@ jobs:
    timeout-minutes: 40
    strategy:
      matrix:
-        edition: [ee, oss]
+        edition: ${{ fromJSON(inputs.only_ee && '["ee"]' || '["ee", "oss"]') }}


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

https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations

https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#excluding-matrix-configurations

nemanjaglumac · 2024-05-16T08:38:29Z

.github/workflows/uberjar.yml

    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 }}


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).

npretto · 2024-05-16T10:46:49Z

.github/workflows/release-embedding-sdk.yml

+        env:
+          BUCKET: ${{ vars.AWS_S3_DOWNLOADS_BUCKET }}
+          BUCKET_PATH: sdk/v${{ inputs.sdk_version }}/metabase.jar
+          OUTPUT_FILE: ./target/uberjar/metabase.jar


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?

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

Fixed in 87b22fc

npretto · 2024-05-16T10:53:06Z

.github/workflows/release-embedding-sdk.yml

+          app-id: ${{ secrets.METABASE_BOT_APP_ID }}
+          private-key: ${{ secrets.METABASE_BOT_APP_PRIVATE_KEY }}
+
+      - name: Create a PR updating readme + published version


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?

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.

WiNloSt · 2024-05-16T11:13:21Z

.github/workflows/uberjar-sdk.yml

@@ -0,0 +1,149 @@
+name: Build + Docker Uberjar for SDK


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.

nemanjaglumac · 2024-05-16T12:09:17Z

.github/workflows/uberjar-sdk.yml

+      - 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 }}


So, if you build from master, this will not push to DockerHub.
Is it the intended behavior?

good point. I need to fix this. Not intended by any mean. This is just a stripdown version of uberjar.yml

nemanjaglumac · 2024-05-16T12:21:15Z

.github/workflows/release-embedding-sdk.yml

+
+      - name: Create a new git tag
+        run: |
+          git tag ${{ env.tag }}


Nit:
Do we intentionally provide only a lightweight tag here?

cc @WiNloSt

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.

nemanjaglumac

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.

WIP some workflows

47b59eb

metabase-bot bot assigned WiNloSt May 10, 2024

metabase-bot bot added the .Team/Embedding label May 10, 2024

WiNloSt added 2 commits May 13, 2024 14:42

Merge branch 'master' into 42498-automate-sdk-release

b54a881

Add reminder todo comment

453e1bf

WiNloSt force-pushed the 42498-automate-sdk-release branch from 06b5cbd to a7c1de2 Compare May 13, 2024 12:13

WiNloSt added 2 commits May 13, 2024 20:33

Complete the workflow

97c06f7

Fix YAML syntax error

c038d2e

WiNloSt force-pushed the 42498-automate-sdk-release branch from a7c1de2 to c038d2e Compare May 13, 2024 13:33

Trigger the workflow

7997bc5

WiNloSt force-pushed the 42498-automate-sdk-release branch from 7997bc5 to c038d2e Compare May 13, 2024 13:48

Fix resuable workflow

97bd6ba

WiNloSt mentioned this pull request May 13, 2024

Trigger the workflow #42572

Closed

WiNloSt added 10 commits May 13, 2024 21:00

Fix uberjar workflow

045964e

Add missing steps before testing

273d6f7

Pass recrets to resuable workflow, so it could run

920cc53

Fix sdk artifact location

7642849

Remove unneeded output

1bbd1bd

fetch tags, so it doesn't fail on push

8930d72

Only running EE uberjar in SDK workflow

d3c3fd8

Because OSS is never needed

Fix incorrect syntax

6f75cdd

Shuffle workflow so it failed fast

2b014db

Fix syntax error

584de89

WiNloSt added 2 commits May 13, 2024 23:01

Attempt to bulid only EE jar

7ec3903

Fix the error handling step

efdef6f

WiNloSt added the no-backport Do not backport this PR to any branch label May 14, 2024

WiNloSt added 3 commits May 14, 2024 16:50

Merge branch 'master' into 42498-automate-sdk-release

4a0b355

Fix failing to upload to s3

d0c32e0

Fix create a PR step

a6d4306

WiNloSt mentioned this pull request May 16, 2024

Implement changelog generation as a part of SDK release process #42750

Open

nemanjaglumac reviewed May 16, 2024

View reviewed changes

WiNloSt removed the request for review from deniskaber May 16, 2024 09:19

WiNloSt added 7 commits May 16, 2024 16:25

Merge branch 'master' into 42498-automate-sdk-release

1b52957

Use a separate uberjar workflow for SDK

4f03563

Remove a bunch of unused steps

ec77bbe

Update variable names, step names

0639cfb

Improve tag error message

0e3b35f

Correct m2-cache-key to be unique

46c0ee9

Fix the push tag failure step to only run when failed to push tag

5df46dd

WiNloSt force-pushed the 42498-automate-sdk-release branch from 78650e3 to 5df46dd Compare May 16, 2024 10:14

npretto reviewed May 16, 2024

View reviewed changes

Fix changed artifact name

025d62c

npretto reviewed May 16, 2024

View reviewed changes

WiNloSt added 2 commits May 16, 2024 17:59

Improve env name for clarity

87b22fc

Fix extra trailing empty line

a4fbb2d

WiNloSt requested review from deniskaber and nemanjaglumac May 16, 2024 11:06

WiNloSt commented May 16, 2024

View reviewed changes

WiNloSt assigned nemanjaglumac and unassigned WiNloSt May 16, 2024

nemanjaglumac reviewed May 16, 2024

View reviewed changes

Remove conditions from running certian steps

f97ed74

nemanjaglumac reviewed May 16, 2024

View reviewed changes

nemanjaglumac approved these changes May 16, 2024

View reviewed changes

Removing npm publish dry-run

81e4907

WiNloSt enabled auto-merge (squash) May 16, 2024 12:39

deniskaber approved these changes May 16, 2024

View reviewed changes

WiNloSt merged commit 3dbeb3b into master May 16, 2024
109 checks passed

WiNloSt deleted the 42498-automate-sdk-release branch May 16, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automate releasing embedding SDK #42516

Automate releasing embedding SDK #42516

WiNloSt commented May 10, 2024 •

edited

github-advanced-security bot commented May 13, 2024

nemanjaglumac left a comment

nemanjaglumac May 16, 2024

WiNloSt May 16, 2024

nemanjaglumac May 16, 2024

nemanjaglumac May 16, 2024

nemanjaglumac May 16, 2024

WiNloSt May 16, 2024

nemanjaglumac May 16, 2024

nemanjaglumac May 16, 2024

WiNloSt May 16, 2024

nemanjaglumac May 16, 2024

nemanjaglumac May 16, 2024

nemanjaglumac May 16, 2024

npretto May 16, 2024

WiNloSt May 16, 2024

WiNloSt May 16, 2024

npretto May 16, 2024

WiNloSt May 16, 2024

WiNloSt May 16, 2024

nemanjaglumac May 16, 2024

WiNloSt May 16, 2024 •

edited

nemanjaglumac May 16, 2024

WiNloSt May 16, 2024

nemanjaglumac left a comment

		commit:
		description: 'Optional full-length commit SHA-1 hash'

	BUCKET: ${{ inputs.bucket }}
	OUTPUT_FILE: ${{ inputs.output-name }}

Automate releasing embedding SDK #42516

Automate releasing embedding SDK #42516

Conversation

WiNloSt commented May 10, 2024 • edited

Description

How to verify

github-advanced-security bot commented May 13, 2024

nemanjaglumac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WiNloSt May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemanjaglumac left a comment

Choose a reason for hiding this comment

WiNloSt commented May 10, 2024 •

edited

WiNloSt May 16, 2024 •

edited