-
Notifications
You must be signed in to change notification settings - Fork 49
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
Cargo dist #2528
Cargo dist #2528
Conversation
6a4b888
to
d47feb8
Compare
4e66c8f
to
0e0575d
Compare
0e0575d
to
6023101
Compare
228f4bd
to
a0f5c13
Compare
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.
Thanks for changing our release infrastructure to use cargo dist @jackkleeman. Not an expert in dist
but from what I can tell, it looks good to me. Would be great if we could try it out before release time to make sure that things are actually working (if there is kind of "dry-run" possible).
@@ -16,6 +19,9 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
app_name: | |||
- restate-cli | |||
- restate-server |
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.
Should we also include restatectl
?
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.
lets address this separately if we want to
on: | ||
pull_request: |
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.
Why is it important to run this workflow on pull requests? Running a release workflow on a pull request is on first sight a bit confusing to me.
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.
Ah ok, I guess we do it because we want to catch breaking changes to dist in a PR, right? If yes, then maybe let's document this somewhere.
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 what dist suggests, and given that we will now run ci as part of the 'plan' stage, which is also what runs on prs, this seems sensible to me. i dont feel strongly about it, but it does mean we have effectively one ci workflow, and release is just about it 'going further' than the pr workflow.
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.
actually, i think i will change this around a bit
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.
ive moved ci to the 'host' stage, so we should still run it deliberately on pr instead of relying on the plan running on PR. but i think its still good for plan to run on each pr, so we know release.yml is still valid, as they suggest.
a0f5c13
to
27a6e74
Compare
8b5f7e2
to
a9441e1
Compare
Anything blocking this from being merged? or should I go ahead and merge it? |
Changes:
restate-{cli,server}.target.tar.xz
instead ofrestate.target.tar.gz
. The tool doesn't support unifying packages together, we would have to make a new cargo package that contains both binary targets, which I'm not really that sure how to do. We could also just manually merge the two tars if people want thisThe following steps are a bit tricky to test, and we may need to debug on next release: