-
-
Notifications
You must be signed in to change notification settings - Fork 580
feat: add network driver options #3302
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3bb9b7f
to
7b5f68c
Compare
Summary by CodeRabbit
WalkthroughAdds support for network driver options by introducing WithDriverOptions(map[string]string), extending NetworkRequest with DriverOptions, documenting the option, and wiring these options into Docker network creation via the Options field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant NET as network.New
participant Builder as NetworkRequest Builder
participant DockerCli as Docker Client
participant Engine as Docker Engine
Dev->>NET: New(..., WithDriverOptions(map[string]string))
NET->>Builder: Apply options
Builder->>Builder: Set DriverOptions on NetworkRequest
Builder->>DockerCli: CreateNetwork(req) with Options = DriverOptions
DockerCli->>Engine: POST /networks/create { Options: {...} }
Engine-->>DockerCli: NetworkID
DockerCli-->>Dev: Result
note over Builder,DockerCli: New/changed: propagate DriverOptions into Docker Options
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
network/network.go (1)
100-107
: Merge driver options when multiple customizers apply.Right now each call to
WithDriverOptions
replaces the entire map. That breaks composition if a library and a caller both want to add their own driver options—the later call wipes out the earlier one. Please merge into the existing map (initializing when nil) so multiple customizers can cooperate.func WithDriverOptions(options map[string]string) CustomizeNetworkOption { return func(original *network.CreateOptions) error { - original.Options = options + if len(options) == 0 { + return nil + } + if original.Options == nil { + original.Options = make(map[string]string, len(options)) + } + maps.Copy(original.Options, options) return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker.go
(1 hunks)docs/features/networking.md
(1 hunks)network.go
(1 hunks)network/network.go
(2 hunks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Before merging, I'd suggest we add a test for this, although the code is pretty simply: it just passes the options to the Docker type.
@mdelapenya The only way to test that I see currently is to use the driver option (I'm not immediately sure when I'll find some time to try this out) |
What does this PR do?
Used the field
Options
innetwork.CreateOptions
(docker).Also added a
DriverOptions
field to the (deprecated)NetworkRequest
since we convert fromCreateOptions
toNetworkRequest
and then back toCreateOptions
in the docker provider.Why is it important?
This gives more control over network creation.
For example it allows to set a container interface prefix with
com.docker.network.container_iface_prefix
.This can give stability to the interface names when needing to inject one in the config (before the container is running) when the container attaches to multiple networks.
Without it there is no way of predicting which interface (
eth0
,eth1
, ...) would correspond with which network.Related issues