-
Notifications
You must be signed in to change notification settings - Fork 76
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
Feature/added ipc flag #285
Conversation
- Introduced a new IPC class that extends RockerExtension to handle IPC namespace options. - Implemented `get_name` to return the 'ipc' identifier. - Added `get_docker_args` method to generate Docker IPC arguments based on CLI input. - Included a `get_preamble` method that returns an empty string as no preamble is needed. This commit enables the use of IPC-related Docker arguments in Rocker configurations.
Add IPC support to RockerExtension
- Introduced a new IPC class that extends RockerExtension to handle IPC namespace options. - Implemented `get_name` to return the 'ipc' identifier. - Added `get_docker_args` method to generate Docker IPC arguments based on CLI input. - Included a `get_preamble` method that returns an empty string as no preamble is needed. This commit enables the use of IPC-related Docker arguments in Rocker configurations.
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.
Thanks for the contribution, I thought that this was going to be more complicated. Overall it looks good. I have a few stylistic changes and a request for adding a full link for the options documentation which is oddly hard to get to directly.
Can you please add a basic test too to make sure that it gets passed through correctly and we keep coverage high. You can use the network test as a good example:
Lines 117 to 147 in 73ab0d6
class NetworkExtensionTest(unittest.TestCase): | |
def setUp(self): | |
# Work around interference between empy Interpreter | |
# stdout proxy and test runner. empy installs a proxy on stdout | |
# to be able to capture the information. | |
# And the test runner creates a new stdout object for each test. | |
# This breaks empy as it assumes that the proxy has persistent | |
# between instances of the Interpreter class | |
# empy will error with the exception | |
# "em.Error: interpreter stdout proxy lost" | |
em.Interpreter._wasProxyInstalled = False | |
@pytest.mark.docker | |
def test_network_extension(self): | |
plugins = list_plugins() | |
network_plugin = plugins['network'] | |
self.assertEqual(network_plugin.get_name(), 'network') | |
p = network_plugin() | |
self.assertTrue(plugin_load_parser_correctly(network_plugin)) | |
mock_cliargs = {'network': 'none'} | |
self.assertEqual(p.get_snippet(mock_cliargs), '') | |
self.assertEqual(p.get_preamble(mock_cliargs), '') | |
args = p.get_docker_args(mock_cliargs) | |
self.assertTrue('--network none' in args) | |
mock_cliargs = {'network': 'host'} | |
args = p.get_docker_args(mock_cliargs) | |
self.assertTrue('--network host' in args) |
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.
Thanks for the cleanup and adding a test. This is great to add as a feature.
get_name
to return the 'ipc' identifier.get_docker_args
method to generate Docker IPC arguments based on CLI input.get_preamble
method that returns an empty string as no preamble is needed.This PR enables the use of IPC-related Docker arguments in Rocker configurations and should close the issue #241.