-
Notifications
You must be signed in to change notification settings - Fork 139
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: combine ci #1312
base: main
Are you sure you want to change the base?
chore: combine ci #1312
Conversation
….yaml, fmt.yaml, clippy-lint.yaml, and add general-check.yaml to combine all in one file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1312 +/- ##
==========================================
- Coverage 19.29% 6.54% -12.75%
==========================================
Files 164 53 -111
Lines 10852 3268 -7584
==========================================
- Hits 2094 214 -1880
+ Misses 8758 3054 -5704
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
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 looking into this. i'am yet to give it a thorough review but can you please fix ci https://github.com/stratum-mining/stratum/actions/runs/12425379179/workflow ?
sure, i do it as a syntax test and forgot to fix |
you think that we can set some pattern to the jobs names? i made just a "draft" to bring to table |
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 anyway to make this shorter?
Can we try to avoid repeating this following part?
runs-on: ${{ matrix.os }}
strategy:
matrix:
os:
- macos-latest
- ubuntu-latest
include:
- os: macos-latest
target: x86_64-apple-darwin
- os: ubuntu-latest
target: x86_64-unknown-linux-musl
steps:
- uses: actions/checkout@v4
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: 1.75.0
override: true
components: clippy
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 going in the right direction but currently it does not seem to be fully working https://github.com/stratum-mining/stratum/actions/runs/12472789699/workflow
You can take some inspiration from here https://github.com/lightningdevkit/rust-lightning/tree/main/ci If you feel the need to move things to a shell script and just invoke it in the GH action. I think that would be also great for local dev env where devs can just call the shell script to replicate the GH action flow. Edit: Maybe this is a bit too much for this PR and can happen in a subsequent one. |
You think that is good? @jbesraa |
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.
Looks great! Added a few sugg's for the naming in order to make them consistent.
I would also create a sub-folder inside scripts
folder and call it workflows
, and also align the files names: build.sh
, clippy.sh
, format.sh
and semver.sh
…ithub inside a workflows directory
Greats sugg's, i hope this help to make the code more clean, and easy to understand to others, and ourselfs too |
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.
Great work on streamlining the CI! I noticed that test.yaml
seems to have been deleted, and it might have been missed. Could you kindly add the corresponding action to it, named ci
? Once that’s done, we’ll be all set.
I think we should change |
I haven't really looked into the contents of the PR, but overall I feel the commit history is getting really dirty we are introducing changes and later adding new commits to fix things from previous commits if we merged this PR as it is, our commit history on I highly recommend to the PR author to study how to do rebase and squash on |
thanks for advise, i'll look into this! |
… lower case, and create bash script to ci job.
….yaml, fmt.yaml, clippy-lint.yaml, and add general-check.yaml to combine all in one file. chore: fix MSRV incorrect job syntax chore: update general-check to be more clean
…stribution_sv2`: remove the `no_std` feature and make them `#![no_std]` as they never use `std` anywhere. - bump their MAJOR version because of feature removal - bump the dependant crates PATCH version - updates docs chore: create shell script to make general-check.yaml more clean. chore: move shell scripts to /scripts chore: fix path to script on general-check.yaml chore: fix path to script on general-check.yaml
chore: update names to a pattern, and also organize bash scripts to github inside a workflows directory chore: adjust wrong clippy.sh file. chore: update general-check.yaml to rust-check.yaml, and job names to lower case, and create bash script to ci job. chore: remove comments, and add perm to test.sh
before foward with PR, can you guys leave a tip to improve the commits like the plebhash said, it just rebase and squash all my commits to only one? @Shourya742 @jbesraa |
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.
Note that by default we want each job to run on both stable and our-msrv versions unless we specify something different, like for cargo fmt
we only use nightly
. Moreover, some of the jobs have OS switch but not others, we should be consistent with this.
Yea I would squash it all into a single commit and add as much context as possible in the commit body while leaving the commit title under ~80 chars.
nice, before forward with pr, i will try to make it, i only have fear to break something and can't recover |
We are here to help, just post any problem you get stuck on |
…d create a new script with examples with new job ci-example.
….yaml, fmt.yaml, clippy-lint.yaml, and add general-check.yaml to combine all in one file. chore: fix MSRV incorrect job syntax chore: update general-check to be more clean
…stribution_sv2`: remove the `no_std` feature and make them `#![no_std]` as they never use `std` anywhere. - bump their MAJOR version because of feature removal - bump the dependant crates PATCH version - updates docs chore: create shell script to make general-check.yaml more clean. chore: move shell scripts to /scripts chore: fix path to script on general-check.yaml chore: fix path to script on general-check.yaml
chore: update names to a pattern, and also organize bash scripts to github inside a workflows directory chore: adjust wrong clippy.sh file. chore: update general-check.yaml to rust-check.yaml, and job names to lower case, and create bash script to ci job. chore: remove comments, and add perm to test.sh chore: remove test.yaml, rust-msrv.yaml, semver-check.yaml, lockfiles.yaml, fmt.yaml, clippy-lint.yaml, and add general-check.yaml to combine all in one file. chore: fix MSRV incorrect job syntax chore: update general-check to be more clean
chore: update names to a pattern, and also organize bash scripts to github inside a workflows directory chore: adjust wrong clippy.sh file. chore: update general-check.yaml to rust-check.yaml, and job names to lower case, and create bash script to ci job. chore: remove comments, and add perm to test.sh chore: add condition to check if directory exist chore: fix path to test.sh chore: try to fix path to test.sh
Hello, as required try to combine all asked yamls in only one yaml to ci/cd separated by jobs.
Issue: #1272