Add ChangeLabelType#271
Conversation
kolyshkin
requested review from
cyphar,
mrunalp,
rhatdan,
runcom and
thaJeztah
as code owners
kolyshkin
force-pushed
the
change-type
branch
2 times, most recently
from
7ca5651 to
f07753b
Compare
| type LabelType int | ||
|
|
||
| const ( | ||
| TypeProcess LabelType = iota + 1 |
There was a problem hiding this comment.
I'm always a bit cautious on using iota for public API; go makes it easy to still allow untyped integers to be used (or to be cast to the right type), so if the order would ever change, that could break uses, and wouldn't be immediately obvious here.
There was a problem hiding this comment.
Do you want to add a test case to check the order hasn't changed? Or would you prefer to not use iota at all?
There was a problem hiding this comment.
Perhaps no iota at all, and just explicitly set the values; although I don't expect it to happen in this repository, I've been bit a few times by cases where values were implicitly changed (e.g. some refactor sorting consts, or someone adding a new consts somewhere in between).
While technically we could consider the consts to be the contract, I have seen situations where values were either hard-coded, or (not sure if that can happen for this one) persisted, so updating the values would (sometimes subtly) break those cases.
Explicitly setting values reduces the temptation to update values as it's more clear that changing them means you're changing the API.
(Hope I make sense 😅)
There was a problem hiding this comment.
You do make sense; yet I still like iota because it's concise. I think we can prevent the values being changed by adding a test case.
Well, after adding a test case it's no so concise (if we count the test as well).
Switched to numbers.
All (open source) users of KVMContainerLabel[s] and InitContainerLabel[s] (containerd, podman, and cri-o) are immediately releasing the acquired MCS. They only need the type field, so they can change the existing label to that type. Everything else, including the just-generated unique MCS label, is not used. Introduce ChangeLabelType which does just what all those users need. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2 participants
All (open source) users of KVMContainerLabel[s] and InitContainerLabel[s] (containerd, podman, and cri-o) are immediately releasing the acquired MCS. They only need the type field, so they can change the existing label to that type. Everything else, including the just-generated unique MCS label, is not used.
Introduce
ChangeLabelTypewhich does just what all those users need.Fixes: #266
Testing this: