cri: add deprecation warnings for mirrors, auths, and configs#9319
cri: add deprecation warnings for mirrors, auths, and configs#9319samuelkarp merged 4 commits intocontainerd:mainfrom
Conversation
| } | ||
| log.G(ctx).Warning("`auths` is deprecated, please use `configs` instead") | ||
| warnings = append(warnings, deprecation.CRIRegistryAuths) | ||
| log.G(ctx).Warning("`auths` is deprecated, please use `ImagePullSecrets` instead") |
There was a problem hiding this comment.
Would it make sense to leave the logging to the caller of this function? Or would that be complicated, because we also want to log the replacement?
(not a blocker, just thinking out loud)
There was a problem hiding this comment.
I hadn't considered that. I think this is easier since the log line message and the deprecation warning message are different, but I don't really have a strong opinion on it.
There was a problem hiding this comment.
Not a blocker as well; existing code already logs, so it's not a new change in this PR.
Mostly planting "food for thought" here; reduce/avoid logging in library packages where it may not be needed by all consumers of it. That said, containerd's "context" logger can already provide more control over logging to the consumer
(but I just realise that I don't think we have a WithoutLogger() utility to disable / remove a logger from a context - perhaps that's something we should think about 🤔)
|
|
||
| if len(c.Registry.Configs) != 0 { | ||
| warnings = append(warnings, deprecation.CRIRegistryConfigs) | ||
| log.G(ctx).Warning("`configs` is deprecated, please use `config_path` instead") |
There was a problem hiding this comment.
Bit orthogonal, and definitely not a blocker (I lost a better reference about this (:disappointed:) that had some great motivations / background on this), but generally "please" should be avoided in messages; here's some references (but perhaps there's better ones);
There was a problem hiding this comment.
I've never considered that before!
I matched the existing messages which had "please". Do you feel strongly enough that we should change this?
There was a problem hiding this comment.
No, not a blocker for this PR at all. I did notice the existing wording, so it made perfect sense to match that (consistency > being perfect), but it's something we could look at separately and check if we have more occurrences like that.
samuelkarp
force-pushed
the
config-deprecation-warnings
branch
from
cb1d572 to
a30c2d1
Compare
samuelkarp
force-pushed
the
config-deprecation-warnings
branch
from
a30c2d1 to
b221457
Compare
|
Rebased again to resolve conflicts with #9321 |
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
samuelkarp
force-pushed
the
config-deprecation-warnings
branch
from
b221457 to
a596d09
Compare
|
Who wants to relief Sam from rebase hell? 😂 ❤️ |
|
I'm just waiting for CI to pass and then put this in the merge queue 😂 |
|
@thaJeztah Mind reviewing after the last rebase so this can merge? |
samuelkarp
added
cherry-pick/1.6.x
cherry-pick/1.7.x
samuelkarp
added
cherry-picked/1.7.x
samuelkarp
added
cherry-picked/1.6.x
Update fork-external main with upstream main @ 452ec25 Related work items: containerd#5890, containerd#7647, containerd#9218, containerd#9233, containerd#9258, containerd#9270, containerd#9274, containerd#9279, containerd#9283, containerd#9286, containerd#9289, containerd#9290, containerd#9294, containerd#9295, containerd#9297, containerd#9305, containerd#9306, containerd#9308, containerd#9316, containerd#9317, containerd#9319, containerd#9320, containerd#9321
4 participants
Part of #9312
These haven't been removed from
mainyet, so opening them here and then will backport.