-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
ci: Update diff checks so that the task actually works #1578
Conversation
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.
I think this looks good. One nit
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.
Light review ACK
@@ -8,6 +9,8 @@ if git checkout HEAD^ && scripts/buildtable.pl >/tmp/table.mediawiki 2>/dev/null | |||
echo "$newdiff" | |||
exit 1 |
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.
Not sure, but maybe seeing exits at this line on the CI and locally that appear to be false negatives on the first run, then green on the next one.
CI: https://github.com/bitcoin/bips/actions/runs/8977001323/job/24831359486#step:3:17
locally:
jon|(19a39e2...):~/bitcoin/bips$ git checkout origin/pr/1557
Previous HEAD position was 19a39e2 Fix links
HEAD is now at 2f620f5 Fix layer
jon|(2f620f5...):~/bitcoin/bips$ ./scripts/diffcheck.sh
Previous HEAD position was 2f620f5 Fix layer
HEAD is now at 1cf5983 Add information about service vendor
+++ /tmp/after.diff 2024-05-10 11:31:38
jon|(1cf5983...):~/bitcoin/bips$ ./scripts/diffcheck.sh
Previous HEAD position was 1cf5983 Add information about service vendor
HEAD is now at 19a39e2 Fix links
README table matches expected table from BIP files
Currently the Diff Checks CI task silently fails. This PR fixes that so it both works and the task will actually fail if the script is failing.
The failure was caused by 2 issues:
buildtable.pl
on the current commitAdditionally,
actions/checkout@v3
is deprecated in favor ofactions/checkout@v4
so this PR bumps that too.Lastly, since diff checks will fail for PRs that do not have numbers yet, I've clarified that this is expected in the CI task's name.