-
Notifications
You must be signed in to change notification settings - Fork 4
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: set ulimit by default in startup strategy #177
fix: set ulimit by default in startup strategy #177
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.
Awesome! Super thanks for the contribution 🙌 i will look at this more later.
@jarlah I made a mistake in my thinking fixed it now. It's building again and more resilient. I might make a new merge request tomorrow that cleans up the code a little bit if you're ok with it :) |
Thats ok. Just tell me when you feel the PR is ready |
it is ready now. Just saw some things in the code that I might wanna clean up. But that should not be part of this. Because I didn't make or change it. And it's your project :) |
OUR project, anyone who contribute to this repo can call it OUR project :) im just a facilitator 🙇 |
cccdffd
to
e09a887
Compare
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.
some questions
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.
the PR looks good. It introduces new tests and fixes existing tests. The changes are understandable. Lets merge this. I will check if it builds and run the tests locally on rootless docker on nixos. WIll release if everything works
fix #176 and set ulimit so startup time is reduced on platforms that support it.
This has been built and tested on an pc running manjaro.