api/types/container: refactor to use strings.Cut, DRY, move tests and fix validation#44379
Conversation
9e80171 to
6d50608
Compare
These types were moved to api/types/container in 7ac4232, but the unit-tests for them were not moved. This patch moves the unit-tests back together with the types. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
6d50608 to
e7d4ff3
Compare
corhere
left a comment
There was a problem hiding this comment.
Per Hyrum's Law there is inevitably going to be someone (likely with a buggy script) who is going to be upset that we "broke" them. We should call this fix out in the changelog since it is technically a breaking API change.
| func containerID(val string) (idOrName string, ok bool) { | ||
| k, idOrName, ok := strings.Cut(val, ":") | ||
| return idOrName, ok && k == "container" | ||
| } |
There was a problem hiding this comment.
A light sprinkling of generics would let you DRY the code a touch more by pushing the string type-cast into the implementation.
| func containerID(val string) (idOrName string, ok bool) { | |
| k, idOrName, ok := strings.Cut(val, ":") | |
| return idOrName, ok && k == "container" | |
| } | |
| func containerID[S ~string](val S) (idOrName string, ok bool) { | |
| k, idOrName, ok := strings.Cut(string(val), ":") | |
| return idOrName, ok && k == "container" | |
| } |
The compiler is sensible enough to monomorphize the function, too.
There was a problem hiding this comment.
Didn't want to go all overboard on DRY 😂 I think I'll keep that for a future exercise;
There was a problem hiding this comment.
Fair. I only really looked into this to sate my curiosity.
The IPCMode type was added in 497fc88, and from that patch, the intent was to allow `host` (without `:`), `""` (empty, default) or `container:<container ID>`, but the `Valid()` function seems to be too relaxed and accepting both `:`, as well as `host:<anything>`. No unit-tests were added in that patch, and integration-tests only tested for valid values. Later on, `PidMode`, and `UTSMode` were added in 23feaaa and f2e5207, both of which were implemented as a straight copy of the `IPCMode` implementation, copying the same bug. Finally, commit d4aec5f implemented unit-tests for these types, but testing for the wrong behavior of the implementation. This patch updates the validation to correctly invalidate `host[:<anything>]` and empty (`:`) types. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
e7d4ff3 to
e7d75c8
Compare
@corhere I added |
hostconfig: move unit tests to api/types/container
These types were moved to api/types/container in 7ac4232 (#18762), but the unit-tests for them were not moved. This patch moves the unit-tests back together with the types.
api/types/container: rewrite tests to use subtests and asserts
api/types/container: use strings.Cut() and DRY
api/types/container: fix validation for UTSMode, UsernsMode, PidMode
The IPCMode type was added in 497fc88 (#9074), and from that patch, the intent was to allow
host(without:),""(empty, default) orcontainer:<container ID>, but theValid()function seems to be too relaxed and accepting both:, as well ashost:<anything>. No unit-tests were added in that patch, and integration-tests only tested for valid values.Later on,
PidMode, andUTSModewere added in 23feaaa ( #9339 / #10080) and f2e5207 (#12667), both of which were implemented as a straight copy of theIPCModeimplementation, copying the same bug.Finally, commit d4aec5f (#14134) implemented unit-tests for these types, but testing for the wrong behavior of the implementation.
This patch updates the validation to correctly invalidate
host[:<anything>]and empty (:) types.