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 all 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.
👌
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.
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.
simplify the name since it seems to be local to each workflow run.
From this doc., the name is default to
artifact
, so we probably don't need to make it super unique with workflow ID, commit hash or whatnot.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.
We checkout this last step with
master
so the created version update PR would only have diff from these 2 files: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.
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.
This is copied over from
uberjar.yml
andworkflow_call
metabase-ee-uberjar
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.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.