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

Support all Network.connect parameters in client.containers.run and client.containers.create #3121

Merged
merged 8 commits into from
Nov 20, 2023

Conversation

Skazza94
Copy link
Contributor

@Skazza94 Skazza94 commented May 7, 2023

In PR #3083, I introduced the support for the network_driver_opt parameter in client.containers run and create.

This PR will further extend this support, by adding all the parameters of Network.connect to the aforementioned methods.

I changed the methods signature a bit, removing the network_driver_opts parameter and adding a dict called network_config that supports all the parameters.

I would kindly request if it possible to release a newer version of dockerpy with this feature merged soon, since I need it to continue developing some Kathará features. Thanks.

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

Hi all,
any news on this PR?

We are waiting for it for a long time 😇

@Skazza94
Copy link
Contributor Author

Hi,
I agree with @tcaiazzi. I opened this PR almost SEVEN months ago and it has not still been merged.

Since last release was in June, I think that it is not productive for anyone to wait so long for a new feature to be merged/released in master. Even more if there are other projects waiting for it and that have some features "paused".

Please, take this as constructive criticism. We extensively use docker-py in Kathará, but having this slow release cycles is detrimental for us.

Thanks,
Mariano.

Copy link
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 this PR and your prior contributions. Having this sit around for so long is unfortunate; if you're still interested I would love to help get this merged and we can do a v7 release along with a few other changes.

Overall it looks good, I'd just like to re-use the EndpointConfig type and make sure we are able to support multiple networks as part of container create.

to the network driver. Defaults to ``None``. Used in
conjuction with ``network``. Incompatible
with ``network_mode``.
network_config (dict): A dictionary containing options that are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an EndpointConfig type:

class EndpointConfig(dict):
def __init__(self, version, aliases=None, links=None, ipv4_address=None,
ipv6_address=None, link_local_ips=None, driver_opt=None,
mac_address=None):

Newer versions of the Moby API (will) allow defining multiple networks on create, so let's keep this the same here, e.g. Dict[str, EndpointConfig], so that we don't have to update this again / make another breaking change.

(I'm ok with this breaking change to replace network_driver_opt since it was just introduced in v6, so it is hopefully not that entrenched in codebases yet.)

@milas milas self-assigned this Sep 29, 2023
@milas milas added this to the 6.x-next milestone Sep 29, 2023
- Renamed parameter from `network_config` to `networking_config` to be more semantically correct with the rest of the API.
@Skazza94
Copy link
Contributor Author

Hi @milas,
no worries! Thanks for the reply.

I've made the requested changes. The parameter has now been updated to be of type Dict[str, EndpointConfig]. Additionally, I have also refactored the sanity checks implemented within _create_container_args to align with the new parameter structure.

Furthermore, I have renamed the new parameter to networking_config, since it is much more semantically coherent with the rest of the API, as the class itself is named NetworkingConfig.

Let me know if you require any other change.

Thanks,
Mariano.

@milas milas merged commit b70cbd0 into docker:main Nov 20, 2023
1 check passed
@milas
Copy link
Contributor

milas commented Nov 20, 2023

Thanks again for making the changes and apologies with how long this took to get merged. I'm going to get Python 3.12 compatibility fixed and release a 7.0.0 ASAP

sthagen pushed a commit to sthagen/docker-docker-py that referenced this pull request Nov 20, 2023
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

3 participants