-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore: improve blockifier CI trigger and scope #76
Conversation
55af520
to
ffba684
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.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1)
.github/workflows/blockifier_coverage.yml
line 0 at r2 (raw file):
note to self:
is this CI phase even needed?
don't we have a codecov phase in the main CI?
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.
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)
.github/workflows/blockifier_ci.yml
line 19 at r2 (raw file):
- 'crates/native_blockifier/**' - 'scripts/install_build_tools.sh' - 'scripts/requirements.txt'
Do we want this requirement file or the one under crates/blockifier/tests
?
Code quote:
- 'scripts/requirements.txt'
.github/workflows/blockifier_compiled_cairo.yml
line 10 at r2 (raw file):
- v[0-9].** paths: - '.github/workflows/blockifier_compiled_cairo.yml'
What does it mean to have the file name in the file itself?
Code quote:
- '.github/workflows/blockifier_compiled_cairo.yml'
ffba684
to
fc475fd
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.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @noaov1)
.github/workflows/blockifier_ci.yml
line 19 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Do we want this requirement file or the one under
crates/blockifier/tests
?
good catch.. this is actually only needed in the blockifier_compiled_cairo
job below
.github/workflows/blockifier_compiled_cairo.yml
line 10 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What does it mean to have the file name in the file itself?
so this job triggers when it changes (I think this is what it does, at least)
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.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)
.github/workflows/blockifier_coverage.yml
line at r2 (raw file):
Previously, dorimedini-starkware wrote…
note to self:
is this CI phase even needed?
don't we have a codecov phase in the main CI?
864a6bd
to
f77c5c4
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.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware)
.github/workflows/blockifier_ci.yml
line 11 at r4 (raw file):
- main-v[0-9].** tags: - v[0-9].**
Why is there no paths here?
Code quote:
push:
branches:
- main
- main-v[0-9].**
tags:
- v[0-9].**
.github/workflows/blockifier_ci.yml
line 27 at r4 (raw file):
- 'build_native_blockifier_in_docker.sh' - 'crates/blockifier/**' - 'crates/native_blockifier/**'
Why is the native blockifier trigger this ci job?
Code quote:
- 'crates/native_blockifier/**'
.github/workflows/blockifier_ci.yml
line 29 at r4 (raw file):
- 'crates/native_blockifier/**' - 'scripts/build_native_blockifier.sh' - 'scripts/install_build_tools.sh'
What is this file?
Code quote:
- 'scripts/install_build_tools.sh'
.github/workflows/blockifier_compiled_cairo.yml
line 6 at r4 (raw file):
push: branches: - main
Why here we don't also have: main-v[0-9].**
?
Do we want to add this branch to the papyrus ci?
Code quote:
branches:
- main
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
- 'crates/blockifier/**' - 'crates/native_blockifier/**' push:
Why here we don't need to mention branches?
Code quote:
push:
.github/workflows/blockifier_post-merge.yml
line 11 at r4 (raw file):
- 'crates/blockifier/**' - 'crates/native_blockifier/**' - 'scripts/requirements.txt'
Why?
Code quote:
- 'scripts/requirements.txt'
f77c5c4
to
6a16258
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @noaov1)
.github/workflows/blockifier_ci.yml
line 11 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is there no paths here?
until we decide how we fix the brokefier and not create many redundant artifacts, we will temporarily build a native blockifier artifact on every merge to a release branch (on push
to branches: main, main-v[0-9].**
).
so, no paths: always trigger this job
.github/workflows/blockifier_ci.yml
line 27 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is the native blockifier trigger this ci job?
for the native-blockifier-artifacts-push
job in this file
.github/workflows/blockifier_ci.yml
line 29 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What is this file?
part of the build-native-blockifier process.
script runs docker -> docker calls script to install deps -> script is install_build_tools
.github/workflows/blockifier_compiled_cairo.yml
line 6 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why here we don't also have:
main-v[0-9].**
?
Do we want to add this branch to the papyrus ci?
- good catch, thanks! added
- yes, which papyrus CIs don't have it?
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why here we don't need to mention branches?
I think code coverage report is really only useful during review; "post-merge coverage report" sounds redundant, WDYT?
.github/workflows/blockifier_post-merge.yml
line 11 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why?
my mistake
need the requirements.txt in the blockifier crate (blockifier/** covers 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.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
.github/workflows/blockifier_ci.yml
line 11 at r4 (raw file):
Previously, dorimedini-starkware wrote…
until we decide how we fix the brokefier and not create many redundant artifacts, we will temporarily build a native blockifier artifact on every merge to a release branch (on
push
tobranches: main, main-v[0-9].**
).
so, no paths: always trigger this job
Got it. Document?
Why not have this job in a native_blockifier_ci.yml
file?
.github/workflows/blockifier_ci.yml
line 29 at r4 (raw file):
Previously, dorimedini-starkware wrote…
part of the build-native-blockifier process.
script runs docker -> docker calls script to install deps -> script is install_build_tools
Document?
.github/workflows/blockifier_compiled_cairo.yml
line 6 at r4 (raw file):
Previously, dorimedini-starkware wrote…
- good catch, thanks! added
- yes, which papyrus CIs don't have it?
I see it's missing in:
papyrus_ci.yml
, papyrus_docker-publish.yml
and helm-install.yml
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
Previously, dorimedini-starkware wrote…
I think code coverage report is really only useful during review; "post-merge coverage report" sounds redundant, WDYT?
I agree. What will adding branches do? Trigger this job post merge to the mentioned branches?
6a16258
to
eb1fda5
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)
.github/workflows/blockifier_ci.yml
line 11 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Got it. Document?
Why not have this job in anative_blockifier_ci.yml
file?
- done
- already have a TODO for that at the top of the file :)
.github/workflows/blockifier_compiled_cairo.yml
line 6 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I see it's missing in:
papyrus_ci.yml
,papyrus_docker-publish.yml
andhelm-install.yml
out of scope of this PR, but we should fix this
eb1fda5
to
583f426
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.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1)
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I agree. What will adding branches do? Trigger this job post merge to the mentioned branches?
oops... this was run on every push to any branch...
deleted the push
event
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.
Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @dorimedini-starkware)
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
Previously, dorimedini-starkware wrote…
oops... this was run on every push to any branch...
deleted thepush
event
Why don't we want it to run on every push to the (native ) blockifier branch?
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)
.github/workflows/blockifier_coverage.yml
line 9 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why don't we want it to run on every push to the (native ) blockifier branch?
what is the native blockifier branch?
if you are asking why not run on every push to main/main-v0.13.2 - why run on every push? all you get is a post-merge report in your inbox; IMO running jobs on push events is only useful (if even) when the failure indicates an error in the code, like test failure or artifact build failure. WDYT?
In any case I think this particular discussion is a bit redundant since codecov should be moving to the main CI anyway
583f426
to
7ed0cdb
Compare
Signed-off-by: Dori Medini <dori@starkware.co>
7ed0cdb
to
52f22f0
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.
Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information
This change is