Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add rust ci #3

wants to merge 1 commit into from

Conversation

JaeAeich
Copy link
Collaborator

@JaeAeich JaeAeich commented Sep 1, 2024

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:

  • Add a CI workflow for running integration tests with Rust, including coverage upload to Codecov.
  • Add a CI workflow for running unit tests with Rust, including coverage upload to Codecov.
  • Add a CI workflow for Clippy lint checks to ensure code quality.
  • Add a CI workflow for testing with minimal dependency versions using Rust.
  • Add a CI workflow to check the minimal supported Rust version (MSRV).
  • Add a CI workflow to check feature flag combinations using cargo-hack.
  • Add a CI workflow to ensure code formatting with rustfmt.
  • Add a CI workflow to check semantic versioning with cargo-semver-checks.

Copy link

sourcery-ai bot commented Sep 1, 2024

Reviewer's Guide by Sourcery

This 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

Change Details Files
Implement integration and unit testing actions
  • Set up integration test action with code coverage reporting
  • Set up unit test action with code coverage reporting
  • Use cargo-llvm-cov for generating coverage reports
  • Upload coverage reports to Codecov
actions/rust/code-test/integration-test/action.yaml
actions/rust/code-test/unit-test/action.yaml
Add code quality checks
  • Implement Clippy lint check action
  • Add formatting check using rustfmt
  • Set up minimal supported Rust version (MSRV) check
  • Implement semantic versioning check using cargo-semver-checks
actions/rust/code-quality/lint/action.yaml
actions/rust/code-quality/format/action.yaml
actions/rust/code-quality/msrv/action.yaml
actions/rust/code-quality/semver/action.yaml
Implement additional testing and maintenance actions
  • Add minimal dependency version test
  • Set up feature flag combination testing using cargo-hack
actions/rust/code-test/minimal-version-test/action.yaml
actions/rust/code-quality/hack/action.yaml

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@JaeAeich
Copy link
Collaborator Author

JaeAeich commented Sep 1, 2024

WIP

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link

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.

Suggested change
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
Copy link

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

Copy link
Member

@uniqueg uniqueg left a 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.

@vemonet
Copy link

vemonet commented Sep 2, 2024

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
Copy link

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

Copy link
Collaborator Author

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:

@uniqueg
Copy link
Member

uniqueg commented Sep 3, 2024

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)

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?

@JaeAeich
Copy link
Collaborator Author

JaeAeich commented Sep 3, 2024

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 observability > speed > redundancy.

Separation gives me observability, for example we have two actions for intergration and unit test (there is a prob here, dicussed below) even though the difference between both of them is just a flag (ie --lib and --tests). What benefit this gives us is, when a PR comes in the GA dashboard we can see what failed without going into logs and also theoretically each job has lesser tests to run so it should be faster (more discussed below).

Redundancy

There are some jobs like test job which should probably not be run separately. For example, for rust CI, I would like to run them on all the OSs, so if I divide a job namely test into unit and integration, and multiple OSs namely macos, windows, ubuntu, this gives use 6 job in total which could have been just 3. Plus for this specific case we would also be uploading 6 reports to codecov per push. Also IDK what this does to our coverage score (comments?).

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.

Speed

Coming 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.

Maintainability

What 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 .github dir for rust and python individually, that repo will have all the actions with all default. This way if we ever need to make any changes, or any CI misbehaves, its not the middleware (.github repo) that needs to change, ie we wont have to ideally touch the .github dir in individual repos all we will have to do is change modify actions, and if we do need to do that we can with a simple pull and merge.

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 .github repo has bbeen updated and if so update create a PR automatically.

@vemonet
Copy link

vemonet commented Sep 3, 2024

Thanks @JaeAeich for the detailed argumentation! I have never used composite actions in the past so my knowledge on them is limited

Also IDK what this does to our coverage score (comments?).

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" :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants