Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automate releasing embedding SDK #42516
Changes from 23 commits
47b59eb
b54a881
453e1bf
97c06f7
c038d2e
7997bc5
97bd6ba
045964e
273d6f7
920cc53
7642849
1bbd1bd
8930d72
d3c3fd8
6f75cdd
2b014db
584de89
7ec3903
efdef6f
4a0b355
d0c32e0
a6d4306
581c53d
7ed56f8
91fbea7
27bbf85
2037edd
d39e5f0
31989fd
9876036
827154c
8f6d3ab
59a2ded
032e512
0d29c66
1b52957
4f03563
ec77bbe
0639cfb
0e3b35f
46c0ee9
5df46dd
025d62c
87b22fc
a4fbb2d
f97ed74
81e4907
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
👌
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
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: wondering if we can cache the checkout here with https://github.com/actions/cache since we checked out twice. let's worry about this later.
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.
answered in https://github.com/metabase/metabase/pull/42516/files#r1601288876
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: if this process is the same as above (
test
), I wonder if we can cache this processThere 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, maybe we could bundle it. Although, the workflow would look cleaner since each job is responsible for certain things.
Edited: It seems our FE and BE preparation are already using cache for some steps, so they're not too slow. I don't think this should be a concern.
Also, I deliberately have
test
separated here because it's a sanity check if we could run the next 2 jobs in parallel (see the above workflow visualization) if we bundlebuild-sdk
withtest
we probably have to do the same withbuild-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.
Does the version gets bumped before this step? I see we bumped the version before creating a PR below, but I don't think we bumped the version of either
package.template.json
or the built artifact inresources/embedding-sdk
. Otherwise the NPM package would be the old versionThere 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, this step is still incorrect as pointed out here. I'll fix this.
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 for me to understand - why do we need to build and upload jar?
I would assume - to make sure clients use specific metabase version that we tested with SDK, 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.
Yes, for now, we say the jar built with the same commit as the SDK would be compatible with the SDK, this could change when we start shipping Metabase v0.50. I think we mentioned we'll then use the jar from the release branch, but I'm not totally 100% sure what commit we'll use in that case.
For more context:
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.
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 4f03563
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 needed, otherwise uberjar workflow won't be able to access
secrets.*
. Alternatively we could specify which secrets we want to pass to it, but we can just useinherit
which passes all secrets.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
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 justFILE
since we're copying files also I think we don't need a bracket just$FILE
should workThere 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
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
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.
Verified locally that regex here works.
I wonder if we should run this as part of the README copy function in
generate-sdk-package-files
though. We currently runcopyFileToOutput("frontend/src/embedding-sdk/README.md", "README.md");
, so we could as easily modify the content there. I guess we do this because we want to bump the version without relying on the script?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 question. I just think of this step as a separate one from generating the package.json which
generate-sdk-package-files.js
handles.However,
generate-sdk-package-files
already accepts an argument as a commit hash, I'm not very familiar with writing CLI, so we could accept named argument instead so I could do--version=<new_version>
without having to pass the commit hash.This could surely become an improvement later on.
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 uses GitHub's own https://github.com/actions/create-github-app-token action rather than a 3rd party one we used in other workflow e.g.
metabase/.github/workflows/auto-assign.yml
Lines 13 to 17 in 0d29c66
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.
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 couldn't find a better way to allow parameterizing
matrix
. Please let me know if there's more ideal way to go about this.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.
Is there a situation where
only_ee
would befalse
? I thought we always only need the EE uberjarThere 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.
For this case, no.
workflow_call
is only being called from the SDK. But I think it's great to be explicit, so folks who care aboutuberjar.yml
could know that it should behave the same by building 2 jars for all existing use cases.Also, this corresponds to
workflow_dispatch
'sinputs.only_ee
which should be evaluated to falsy value sinceonly_ee
isn't defined inworkflow_dispatch
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 a way to parameterize matrix. This is needed because when we build the SDK, we only want to build the EE 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 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
andexclude
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.
Refactor
github.event.inputs
toinputs
most of the time they are equivalent forworkflow_dispatch
exceptinputs
preserve the boolean value..However, the main thing is to make it consistent with
workflow_call
since onlyinputs
is available forworkflow_call
notgithub.even.inputs
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
onmaster
, and by the time this workflow kicks in someone pushed another commit. In that case the uberjar workflow will build from commitdef
.This is not hard to imagine given the frequency we push to
master
(especially just prior to the release).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 a simple sanity check for SDK, we have more tests when running the uberjar workflow already.