CI: Only run release job for release PRs #1758
Conversation
contrib/release.sh
Outdated
| if git log --patch --reverse master.. | grep --quiet "^\+\+\+ b/$crate/Cargo.toml"; then | ||
| if git log --patch --reverse master.. | grep --quiet "^\+version"; then |
There was a problem hiding this comment.
This logic could lead to false positives if there are changes to one crate's manifest but not the version field and the version field is updated in another manifest - does this matter? Someone want to take flex their shell-fu?
There was a problem hiding this comment.
You can filter git log by file, so you can one-line this (and then drop the if since it looks like you're just forwarding the return value anyway):
release_changes() {
git log --patch --reverse master.. -- $crate/Cargo.toml | grep version || true
}
The || true is to prevent set -e from killing the script when this fails. I'd also drop the --quiet from grep. If there are changes it might be nice to see exactly what they are.
There was a problem hiding this comment.
Oops, || true will not forward the return value. Maybe we do need an if here.
There was a problem hiding this comment.
oooh, -- <file> is what I was looking for - nice! Will re-work
There was a problem hiding this comment.
Since, this is only for CI, you can also query crates.io to get the latest version. For the current PR, you can use env variables that cargo sets for you. See: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates
There was a problem hiding this comment.
No strong preference, just suggesting another alternative.
There was a problem hiding this comment.
On further thought, maybe we don't need the if, since we are calling release_changes in an if. I'm not sure.
There was a problem hiding this comment.
We don't seem to need the || true. We now have:
release_changes() {
local crate=$1
git log --patch --reverse master.. -- $crate/Cargo.toml | grep version
}|
I'm not sure if @apoelstra or @sanket1729 are requesting changes or ok with this as is? |
|
Marking high-priority because a bunch of open PRs are failing CI and this hopefully fixes them. |
|
@tcharding I'd like the |
|
Sure thing, I'll hack it up. |
Currently we run the release jobs for all PRs, this leads to red CI runs any time we have unreleased changes in one crate used by another i.e., basically days after a release comes out. Add a `release.sh` script that has more smarts to try and figure out if the patch series currently ontop of tip of mater contains changes that imply its a release PR. To do so we check for changes to the version field in the manifest of each crate.
3335c1f to
e223cbe
Compare
|
Cool! Looks great. |
|
Can we get this one in crew, there is red in every other open PR that this one fixes. |
|
ACK e223cbe |
|
Can you click the "approve" button too please @stevenroose, silly redundancy I know but we need both two ACKS and two "approves" to merge. Well at least I think we do, please correct me if I'm wrong @apoelstra? |
|
Yeah, we need two actual ACKs, then the closest thing we can get Github to do is to enforce "two clicks of the green button". I suppose we could drop the Github requirements and just have maintainers manually enforce the rules. |
Currently we run the release jobs for all PRs, this leads to red CI runs any time we have unreleased changes in one crate used by another i.e., basically days after a release comes out.
Add a
release.shscript that has more smarts to try and figure out if the patch series currently ontop of tip of mater contains changes that imply its a release PR. To do so we check for changes to the version field in the manifest of each crate.