-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
replace travis with Github Actions #1432
Conversation
14b041b
to
76c1a08
Compare
Hey @kallewoof Is there a reason why Cirrus (not super well known CI system) was chosen instead of Github Actions (which is more widely known and would seem a more natural choice given the code is already hosted on GitHub)? |
@augustoproiete Only reason being that bitcoin core is using it. Playing around with Github Actions a bit to see which one is better. |
6dc874d
to
c550c95
Compare
Github Actions seems better. Thanks for the nudge! |
c550c95
to
6b559e6
Compare
6b559e6
to
cbd78b0
Compare
ACK Works as expected |
Nit: Having all the checks in one job means that if the first check fails, you don't know if the 2nd or 3rd one fails, so sometimes you get a few extra back and forth "nope, try again." and can be annoying as a maintainer. Other than that, ACK. |
@junderw That's not the case with GitHub actions... You can drilldown and see exactly which step of the job failed, along with debug/verbose info written to the console by the build script: e.g. https://github.com/augustoproiete-forks/bitcoin--bips/actions/runs/4361581418/jobs/7625593905 |
Hum. I think @junderw is correct here, actually. Initially I had errors in the first script and that failed (edit: the remaining scripts did not execute, it looked like). I probably shouldn't use |
We had a similar decision between Cirrus and Github Actions for rust-bitcoin. My understanding is that Core chose to eschew Github Actions because of an abundance of caution around Github's security controls (which IIRC had been in the news recently), and in general, Github being the canonical source of truth for the repo so there's the potential for security issues with GA to be much more serious. What we landed on was "this is probably reasonable paranoia for the reference implementation of Bitcoin's consensus logic, but probably not worth the extra hassle for other libraries, since GA is so convenient". I'd say the BIPs repo probably has the same calculus. It'd be pretty surreal if there was a security issue in GA that led to a BIP text being changed, then somehow nobody noticing for the months it took for implementations to appear and be deployed. |
@kallewoof @junderw Oh, I see now that I misread @junderw's comment - I thought he was talking about the ability to see which step failed, and not about continuing the execution when one of the steps fail. It seems moving the execution of |
Exactly. GA is perfectly fine for BIPs repo imo. Also, I could have been more explicit. By running in parallel, all tests are run. But in sequence, if the first one fails, the 2nd and 3rd one are not run so you only learn their status after fixing the first one and trying again. If parallel, you can see that all 3 are failing and fix all 3 in one commit. |
@junderw So like 685f383 ? (e.g. https://github.com/kallewoof/bips/actions/runs/4370385560 ) Looks a bit redundant, but it did indeed run the tests in parallel. |
@apoelstra Thanks for the feedback! Sounds like GA is the way to go, |
LGTM! ACK |
This ports the CI to use Github Actions on pull requests and pushes. Unfortunately it looks like you have to merge this before it takes effect. You can see the runs on my repository at https://github.com/kallewoof/bips/actions though.
For posterity, I initially did a port to Cirrus before it was pointed out that Github Actions may be better.