Skip to content
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

Merged
merged 7 commits into from
Sep 3, 2024
Merged

Conversation

sachinkum0009
Copy link
Contributor

  • 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 PR enables the use of IPC-related Docker arguments in Rocker configurations and should close the issue #241.

- 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.
…c-flag"

This reverts commit 27cbd58, reversing
changes made to 73ab0d6.
- 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.
Copy link
Collaborator

@tfoote tfoote left a 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:

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)

src/rocker/extensions.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
src/rocker/extensions.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tfoote tfoote left a 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.

@tfoote tfoote merged commit 18c2cc4 into osrf:main Sep 3, 2024
4 checks passed
@tfoote tfoote mentioned this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants