-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add rust ci #3
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request adds a comprehensive Rust CI setup, introducing various GitHub Actions for code testing, quality assurance, and maintenance. The changes focus on implementing a robust continuous integration pipeline for Rust projects. File-Level Changes
Tips
|
WIP |
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.
Hey @JaeAeich - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
||
- name: Run cargo hack | ||
shell: bash | ||
run: cargo hack --feature-powerset check |
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.
suggestion (performance): Consider using --each-feature for large feature sets
If your project has many features, using --feature-powerset
could generate a large number of combinations, potentially impacting CI time. Consider using --each-feature
instead if this becomes an issue.
run: cargo hack --feature-powerset check | |
run: cargo hack --each-feature check |
components: rustfmt | ||
|
||
- name: Run cargo-semver-checks | ||
uses: obi1kenobi/cargo-semver-checks-action@v2 |
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.
suggestion: Add a comment explaining the semver check
Consider adding a comment explaining what the cargo-semver-checks action does. This would improve the self-documentation of the workflow for maintainers who might not be familiar with it.
- name: Run cargo-semver-checks
# Checks for unintentional breaking changes in public API
uses: obi1kenobi/cargo-semver-checks-action@v2
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 good to me, but perhaps have people double check that are proficient in Rust, e.g., @pavelnikonorov, @vemonet and @aaravm.
Looking good, apart maybe that those actions could be factorized as one? If we always plan to run the fmt check + clippy lint + semver check + msrv + cargo-hack, why not just making it 1 action? This would make it much easier to use and maintain (as a dev I would not like to have to find out the 5 actions to run and to have to set them 1 by 1, I'd rather have 1 action that I know will run all the required checks and flag the errors) You could even make it an action that runs all checks by default with args to disable some checks when the user want Also it would be nice to have a bit of documentation explaining how to use those actions in other repo Github actions workflows There is also "cargo deny" you might want to check: https://github.com/EmbarkStudios/cargo-deny there is a bit of configuration to do but it's nice to check your dependencies and their licenses (to make sure your crate is not using dependencies with licenses that does not fit) |
- name: Install cargo-hack | ||
uses: taiki-e/install-action@cargo-hack | ||
|
||
- name: Run cargo hack |
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.
What is this effectively doing? Even by quickly checking their readme it's not clear https://github.com/taiki-e/cargo-hack
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.
Its is supposed to:
- Ensure that proj compiles with each feature individually enabled.
- Detects missing imports/dependencies
I am currently adding all other ations as well that make sense, there is a possibility that some might have overlaping functionality, that we can decide once this PR is ready and tested with all other actions as well.
Lemme know know if there are anything I'm missing:
- cargo-deny
- miri
- loom?
- os-check (build and test in all envs)
Good point about merging multiple related actions (e.g., one for everything related to static code analysis, one for everything related to testing, etc). But possibly that would reduce the visibility, i.e., it might be harder for users to tell immediately what went wrong. It might also be harder to make checks run in parallel? @JaeAeich: What do you think? What would that look like in practice? Do you see a way to merge actions while still remaining visibility and parallelization? |
Thanks for the review, this is still a WIP (but a review was needed), what my plan with this PR and in conjuntion with python-ci is Separation gives me observability, for example we have two actions for RedundancyThere are some jobs like test job which should probably not be run separately. For example, for I can easily solve the codecov issue by creating a separate action for coverage specifically, but if I merge unit and integration test, we lose observabilty. SpeedComing to the point of adding all of these into a single actions? if I add all of them into single composite workflow, they will run sequentially, as there is no such thing as job in composite workflow (Id love to hear if there is a workaround). So in order to have parallelism and inturn quicker ci this is the way to go IMO. But yeah there surely would be a point of diminishing returns, so I would like to test these CI (as I did for python) to see what is the diff (with caching and current CI, I was able to drop CI time by ~50% for python). So if the diff comes sub 10s I think we can merge some of them def. MaintainabilityWhat I would like to do ideally (if we reach the same concensus), is that repos don't manage and use these actions individually, these composite actions should provide an API for the check that needs to be done and then there should a middleware that should manage all of those. And the workflow that I found which works like that is using git's merge feature. After the CI is actions here are done, we create repos with only git remote add ci https://github.com/elixir-cloud-aai/.github-python-repo.git
git fetch ci
git merge --allow-unrelated-histories ci/main We can also add a CI with a cronjob which would check after sometimes if the |
Thanks @JaeAeich for the detailed argumentation! I have never used composite actions in the past so my knowledge on them is limited
From my experience, I am almost sure codecov is taking care of automatically merging the reports (you just upload it in each branch of the matrix (e.g. macos and ubuntu), and if some parts of the code are only triggered in the tests on macos or ubuntu they will be properly be shown as covered For the speed point, I would say for some of the actions, in particular the static code analysis ones (msrv, fmt check, lint check), they will be so fast to execute that it's not going to be significative and it will be fine to run them sequentially. But indeed for longer running tests you will want them to run in parallel Your plan for maintainability and observability looks sound, would love to see it in "action" :) |
Summary by Sourcery
Introduce multiple CI workflows for Rust projects, including integration and unit testing, code quality checks, and versioning checks. These workflows ensure comprehensive testing and code quality maintenance using various Rust tools and actions.
CI: