Add support for ulimit flag #291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tfoote
requested changes
Oct 16, 2024
| def setUp(self): | ||
| self._instance = Ulimit() | ||
|
|
||
| def _is_arg_translation_ok(self, mock_cliargs, expected): |
There was a problem hiding this comment.
Thanks for comprehensive tests. Could you change this function to return a tuple of the success and message as a tuple that can then be passed to assertFalse or assertTrue so that the message gets captured in the unit test report not just in the console output.
The return would be (is_ok, message_string)
test/test_ulimit.py
Outdated
| """Test single soft limit argument.""" | ||
| mock_cliargs = ["rtprio=99"] | ||
| expected = " --ulimit rtprio=99" | ||
| self.assertTrue(self._is_arg_translation_ok(mock_cliargs, expected)) |
There was a problem hiding this comment.
Suggested change
| self.assertTrue(self._is_arg_translation_ok(mock_cliargs, expected)) | |
| self.assertTrue(*self._is_arg_translation_ok(mock_cliargs, expected)) |
With the addition of the tuple return of the helper method this will expand the results into the assertTrue arguments.
Thanks for the feedback! I've addressed the requests on the latest commit. |
tfoote
approved these changes
Oct 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, this PR adds support for the
ulimitflag (https://docs.docker.com/reference/cli/docker/container/run/#ulimit). This flag is useful when running ROS 1 containers in machines with highnofilelimits (default in Arch Linux) since not limiting this causes system crashes (please read https://answers.ros.org/question/336963/rosout-high-memory-usage/ for more info).This PR adds:
Ulimitclass that extendsRockerExtensionto add support for the--ulimitflag in Dockerhelpcommand and when raising argument type exception (similar to theVolumeextension)