Conversation
| return true | ||
| } | ||
| } | ||
| c.log("Pod is not ready: %s/%s", pod.GetNamespace(), pod.GetName()) |
There was a problem hiding this comment.
I choose a capital letter for the first letter because it represent the resource.
8ee9bb3 to
91df876
Compare
|
first thank you for the PR, I do have a question it isn't directed at you but to everyone else reviewing if it makes sense to change "name" to "Job" or the respective object. I am sure @mattfarina and @TerryHowe have opinions. For future reference please tag me in any PR when it comes to logging, it is my goal to get this in for Helm 4. |
I tink it's a good idea, but I am wondering how it could render on classic helm debug logs with similar line starting content. How it will be formatted?
Yes ! For sure. 🙏 |
|
Like if someone was searching for the structured log for a job named "blah" it might make sense to call this for what it is not just a name. |
|
Hello @robertsirc We are using the helm-sdk and we are consuming debug log by having a custom DebugLog here. I'm wondering how this will work with slog ? We should probably keep the log setup but instead having a custom interface that accept slog. So we could pass a custom one if needed to keep the same behavior. I think my proposal is not correct. Thanks 😊 |
TerryHowe
left a comment
There was a problem hiding this comment.
Generally looks good, just wondered what people thought about updating the log message content a bit.
pkg/kube/ready.go
Outdated
| slog.Debug("Deployment is not ready: observedGeneration does not match spec generation", | ||
| "namespace", dep.Namespace, | ||
| "name", dep.Name, | ||
| "observedGeneration", dep.Status.ObservedGeneration, | ||
| "specGeneration", dep.ObjectMeta.Generation) |
There was a problem hiding this comment.
From the customer's perspective, I wonder if something like this would make more sense:
| slog.Debug("Deployment is not ready: observedGeneration does not match spec generation", | |
| "namespace", dep.Namespace, | |
| "name", dep.Name, | |
| "observedGeneration", dep.Status.ObservedGeneration, | |
| "specGeneration", dep.ObjectMeta.Generation) | |
| slog.Debug("Deployment generation not updated", | |
| "namespace", dep.Namespace, | |
| "name", dep.Name, | |
| "actualGeneration", dep.Status.ObservedGeneration, | |
| "expectedGeneration", dep.ObjectMeta.Generation) |
pkg/kube/ready.go
Outdated
| slog.Debug("Deployment is not ready: not enough pods ready", | ||
| "namespace", dep.Namespace, | ||
| "name", dep.Name, | ||
| "readyReplicas", rs.Status.ReadyReplicas, | ||
| "expectedReplicas", expectedReady) |
There was a problem hiding this comment.
Again, from the customer perspective, I wonder if this would make more sense
| slog.Debug("Deployment is not ready: not enough pods ready", | |
| "namespace", dep.Namespace, | |
| "name", dep.Name, | |
| "readyReplicas", rs.Status.ReadyReplicas, | |
| "expectedReplicas", expectedReady) | |
| slog.Debug("Deployment does not have enough pods ready", | |
| "namespace", dep.Namespace, | |
| "name", dep.Name, | |
| "readyReplicas", rs.Status.ReadyReplicas, | |
| "expectedReplicas", expectedReady) |
See: helm#30698 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
So, we would like to remove how we are doing this logging with |
|
Thanks @robertsirc for your answer. I will adapt my work to follow your choice.
I think it will still be possible to a custom handler function passed to slog. |
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
91df876 to
ef03e2b
Compare
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
4eff8b3 to
28305b5
Compare
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
See: helm#30698 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
|
In my attempt to make changes suggested in #30698 (comment), to have a fully functionning code I had to go much much further. So I've updated the migrated pull request. See: #30708 cc @robertsirc |
See: helm#30698 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
What this PR does / why we need it:
While working on #30685 it was mentioned that we should migrate to slog following this work #30603. To simplify the initial contribution, I think it's a good idea to first migrate
ready.goto slog, then do the contribution.This is a first attempt to migrate. I am not sure what do you want to do with log level. Everything as debug log ?
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)