-
Notifications
You must be signed in to change notification settings - Fork 193
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
fix: docker CI performance & release builds #2659
Conversation
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2659/docs/iroh/ Last updated: 2024-08-21T22:26:22Z |
looks good, no idea if this actually does the thing 😅 |
As far as I've tested, should be good. Tagged in @flub jic. |
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.
would be nice if that 3s sleep could be done better, but i'm not going to block this because of it
|
||
- name: Run Docker image & stats test | ||
run: | | ||
docker run -p 9090:9090 -p 4919:4919/udp -Pd n0computer/iroh-test:latest --rpc-addr 0.0.0.0:4919 start | ||
cargo run --bin iroh -- --rpc-addr 127.0.0.1:4919 stats | ||
# Give the server time to start | ||
sleep 3 |
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 predict a future flake :(
Not sure what you can do better though, try to connect to the rpc addr in a loop with a short sleep and overall timeout of 30s? your connect would have to fail quickly in an identifiable way for that to work though.
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.
If this gets flakey we probably should deal with it asap as a 3 sec startup is really not good.
Also the "old" setup just relied on cargo run
to take it's sweet time compiling.
I agree, but it would require either horrible parsing hacks that listen to the docker logs or actually implementing a /healthz route on the iroh cli. |
## Description - We now build the docker test CI jobs in a way that more closely resembles the release process. It should compile the binaries and build the image significantly faster now. (down from 8+ min to 2 min) - Secrets are properly passed in to the reusable workflow so [release docker builds should no longer fail](https://github.com/n0-computer/iroh/actions/runs/10473026816/job/29005092378) ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist - [x] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.
Description
Breaking Changes
Notes & open questions
Change checklist