feat: Container network configuration support #3983
Conversation
| * {@link ContainerNetworkConfiguration}. | ||
| * | ||
| * @return | ||
| * @since 2.7 |
There was a problem hiding this comment.
The @since tag should be set to the version of the org.eclipse.kura.api bundle that introduces the new API, so I think it should be 2.4 here. Probably the tag should be changed also in other places (all of the new APIs that have been introduced since Kura 5.1.0 release).
|
@nicolatimeus should I modify the meta-type UI? I feel like a dropdown is better suited, but you lose the ability to parameters like what is needed for the container mode. |
|
I think it can depend on which options are the most frequently used. If the enum variants in the list box are the most frequently used this will improve user experience and make things less error prone. If the custom one is the most frequently used this might clutter the ui with no additional benefits. |
|
Are the network types that would be put in the list box Docker-specific or standard (e.g. OCI) or supported also by other engines? |
|
|
||
| public static final class ContainerNetworkConfigurationBuilder { | ||
|
|
||
| private String networkMode = "bridge"; |
There was a problem hiding this comment.
I'm not sure that using this default is ok, setting it might introduce a change in behavior since the previous version, which did not configure a network mode explicitly.
If we use it we must be sure that bridge is the default for this parameter for all container engines.
Otherwise we could use the empty string as a default value or maybe changing the type of ContainerNetworkConfiguration.networkMode to Optional<String>, update public APIs accordingly and keeping Optional.empty() as default.
A Optional.empty() value means that the implementation must not supply the network mode parameter to the container engine, relying on its defaults (the current behavior).
There was a problem hiding this comment.
This is an excellent point. By default, containers do use the bridge configuration, but it probably makes more sense to leave it empty as per the default. Implementing these suggested changes.
|
|
||
| public static final class ContainerNetworkConfigurationBuilder { | ||
|
|
||
| private String networkMode = "bridge"; |
There was a problem hiding this comment.
I'm not sure that using this default is ok, setting it might introduce a change in behavior since the previous version, which did not configure a network mode explicitly.
If we use it we must be sure that bridge is the default for this parameter for all container engines.
Otherwise we could use the empty string as a default value or maybe changing the type of ContainerNetworkConfiguration.networkMode to Optional<String>, update public APIs accordingly and keeping Optional.empty() as default.
A Optional.empty() value means that the implementation must not supply the network mode parameter to the container engine, relying on its defaults (the current behavior).
| @@ -101,6 +104,7 @@ public ContainerInstanceOptions(final Map<String, Object> properties) { | |||
| this.registryUsername = REGISTRY_USERNAME.getOptional(properties); | |||
| this.registryPassword = REGISTRY_PASSWORD.getOptional(properties); | |||
| this.imageDownloadTimeout = IMAGES_DOWNLOAD_TIMEOUT.get(properties); | |||
| this.containerNetworkingMode = CONTAINER_NETWORKING_MODE.get(properties); | |||
There was a problem hiding this comment.
I think you can use Property.getOptional() here
There was a problem hiding this comment.
Fancy, okay will give it a shot.
| /** | ||
| * | ||
| * returns the network mode a container will be created with. supported modes | ||
| * include: 'bridge', 'none', 'container:', 'host'. |
There was a problem hiding this comment.
Maybe just: Returns the network mode a container will be created with (e.g. 'bridge', 'none', 'container:', 'host').
There was a problem hiding this comment.
Good suggestion, changes made!
Allows the user to specify a network driver when creating a container.
From the docker API:
NetworkMode - Sets the networking mode for the container. Supported standard values are: bridge, host, none, and container:<name|id>. Any other value is taken as a custom network’s name to which this container should connect to.
TLDR: add support for NetworkMode Parameter.