runconfig/errors: split ErrConflictHostNetwork#49605
runconfig/errors: split ErrConflictHostNetwork#49605robmry merged 1 commit intomoby:masterfrom br3ndonland:runconfig/errors/split-ErrConflictHostNetwork
ErrConflictHostNetwork#49605Conversation
| // ErrConflictHostNetwork conflict from being disconnected from host network or connected to host network. | ||
| ErrConflictHostNetwork validationError = "container cannot be disconnected from host network or connected to host network" | ||
| // ErrConflictConnectToHostNetwork error when attempting to connect a container to host network when not in host network mode | ||
| ErrConflictConnectToHostNetwork validationError = "cannot connect container to host network - container must be created in host network mode" | ||
| // ErrConflictDisconnectFromHostNetwork error when attempting to disconnect a container from host network when in host network mode | ||
| ErrConflictDisconnectFromHostNetwork validationError = "cannot disconnect container from host network - container was created in host network mode" |
There was a problem hiding this comment.
Not a problem with this pull request, but we really need to look at removing these exported errors; these errors are not used as special (sentinel) errors, and only defined out of convenience; looks like the only place they're used outside of this package is in some tests (for string-matching), which ideally would check for the errdefs type ("invalid parameter" or "conflict", depending on what applies).
I don't think this pull request makes the situation much worse though, so probably not a problem to add on/two more.
I've done a quick check to see if anyone outside our own codebase is using the ErrConflictHostNetwork error, but it doesn't appear to be, so removing that const shouldn't be a problem AFAICS; https://grep.app/search?q=.ErrConflictHostNetwork
|
The change looks good. I'm not sure what's going on with the tests though, haven't seen that failure in Separately, and not essential - it might be worth force-pushing a commit with a modified commit comment, dropping the |
Split the `ErrConflictHostNetwork` error into two distinct errors: 1. `ErrConflictConnectToHostNetwork` when attempting to change the network mode of a running container from a different mode to `host` 2. `ErrConflictDisconnectFromHostNetwork` when the network mode of a running container is `host` and attempting to disconnect from `host` This commit clarifies error messaging by differentiating between the two errors, making it clearer which operation failed and how to fix it. Signed-off-by: Brendon Smith <bws@bws.bio>
|
Thank you both for your feedback. Yes, would appreciate some help on the (possibly unrelated) test failures.
Good call - removed ref from Git commit message and force-pushed. |
robmry
left a comment
There was a problem hiding this comment.
LGTM - thank you.
Pushing the new commit seems to have restarted enough things enough to sort out the tests. I'll go ahead and merge ...
- What I did
Fixes #49604
Split the
ErrConflictHostNetworkerror into two distinct errors:ErrConflictConnectToHostNetworkwhen attempting to change thenetwork mode of a running container from a different mode to
hostErrConflictDisconnectFromHostNetworkwhen the network mode of arunning container is
hostand attempting to disconnect fromhostThis commit clarifies error messaging by differentiating between the two
errors, making it clearer which operation failed and how to fix it.
- How I did it
runconfig/errors.go.daemon/container_operations.goto point to the new errors.integration-cli/docker_cli_network_unix_test.goto assert on the new errors.- How to verify it
Run the updated tests in
integration-cli/docker_cli_network_unix_test.go.- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged) 🐳