Skip to content

Add network_driver_opt to client.containers run and create#3083

Merged
milas merged 2 commits intodocker:masterfrom
Skazza94:master
Jan 27, 2023
Merged

Add network_driver_opt to client.containers run and create#3083
milas merged 2 commits intodocker:masterfrom
Skazza94:master

Conversation

@Skazza94
Copy link
Copy Markdown
Contributor

@Skazza94 Skazza94 commented Dec 27, 2022

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.

@milas milas self-assigned this Jan 26, 2023
milas
milas reviewed Jan 26, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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! 🙃

@Skazza94
Add network_driver_opt to client.containers run and create (#2896)
c5dc2c8 
Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
@Skazza94
Fix docs
c647831 
Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
@Skazza94
Copy link
Copy Markdown
Contributor Author

Skazza94 commented Jan 27, 2023

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.

@milas milas merged commit ee9151f into docker:master Jan 27, 2023
@Skazza94 Skazza94 mentioned this pull request May 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aiordache aiordache Awaiting requested review from aiordache

@ulyssessouza ulyssessouza Awaiting requested review from ulyssessouza

1 more reviewer

@milas milas milas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@milas milas

Labels

kind/enhancement level/dockerclient

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Skazza94 @milas