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

Add network_driver_opt to client.containers run and create #3083

Merged
merged 2 commits into from Jan 27, 2023
Merged

Add network_driver_opt to client.containers run and create #3083

merged 2 commits into from Jan 27, 2023

Conversation

Skazza94
Copy link
Contributor

This pull request adds an additional parameter to client.containers.run and client.containers.create called network_driver_opt.

This parameter allows to specify a dict which allows to pass to the driver some custom values, and it is the same already present in Network.connect.

I requested this feature in #2896, but it was never implemented.

I think it could be useful to also export other parameters of networking_config.

Copy link
Member

@milas milas 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 PR! This looks good to me, I just need you to sign-off on the commits before I can accept it. There's instructions at https://github.com/docker/docker-py/pull/3083/checks?check_run_id=10326470472 if you're unfamiliar with the process.

Also, I agree re: exposing more of networking_config. It doesn't feel great to piecemeal expose parts of it, but this is consistent with the other methods (e.g. Network::connect()) as you pointed out, so let's go with it! 🙃

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
@Skazza94
Copy link
Contributor Author

Hi @milas,
thanks for reviewing the PR 😄. I signed the commits.

Also, I agree re: exposing more of networking_config. It doesn't feel great to piecemeal expose parts of it, but this is consistent with the other methods (e.g. Network::connect()) as you pointed out, so let's go with it! 🙃

I agree that it is consistent with the Network.connect method. As soon as I have time, I will also export the remaining network parameters to client.containers run and create.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants