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

Add running cargo-semver-checks on rust-libp2p to CI #180

Merged
merged 6 commits into from
Nov 15, 2022

Conversation

staniewzki
Copy link
Collaborator

@staniewzki staniewzki commented Nov 14, 2022

PR to solve #178

As mentioned in #178, there should be some support for running checks on multiple crates in CI. I think that that could be adjusted in a follow up PR by using a matrix, as it would be easier to do when a specific crate is given, especially since there can be some dependencies needed (like protobuf-compiler in rust-libp2p)

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
@staniewzki staniewzki changed the title Add running cargo-semver-checks on rust-libp2p Add running cargo-semver-checks on rust-libp2p to CI Nov 14, 2022
Copy link
Owner

@obi1kenobi obi1kenobi 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! Broadly, it just would be nice to have some more documentation (in the form of comments) describing what issue this regression test protects against, how the specific project and commit were chosen, etc. That sort of thing will be useful to us in the future when adding more regression / integration tests like this, as well as if one day this regression test fails and we need to figure out what went wrong. Because I promise that a year from now, none of us will remember what the problem was that made us want to add this test, and those docs will be all we have left to remind us.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
with:
persist-credentials: false
repository: 'libp2p/rust-libp2p'
ref: '3371d7ceab242440216ae9ab99829631fa418f3b'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider leaving some documentation (as comments near here and near the top of the job) describing why this particular commit is interesting and worth testing?

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>
@obi1kenobi obi1kenobi merged commit 8b95445 into obi1kenobi:main Nov 15, 2022
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.

2 participants