Closed cayla closed 11 months ago
Hey, thanks for the PR. This comment would fire for every PDB until a matching one is found, and can fire even if a PDB ends up getting found by later iterations of the loop.
Could you move this comment down to just before return false, nil
, and only set this comment if any PDB has been ignored to mismatched namespaces?
Something like:
func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, labels map[string]string) (bool, error) {
var hasNamespaceMismatch bool
for _, budget := range budgets {
if budget.Namespace() != namespace {
hasNamespaceMismatch = true
continue
}
selector, err := metav1.LabelSelectorAsSelector(budget.PodDisruptionBudgetSelector())
if err != nil {
return false, fmt.Errorf("failed to create selector: %w", err)
}
if selector.Matches(internal.MapLabels(labels)) {
return true, nil
}
}
if hasNamespaceMismatch {
score.AddComment("", "PodDisruptionBudget not found", "Cannot associate the PodDisruptionBudget to the resource because they are not explicitly in the same namespace")
}
return false, nil
}
Thank you for your feedback. If you can't tell, I am not a proper dev, let alone a go dev. But i am trying to give back more lately even if it means I goof up now and then.
I took your advice on getting the output outside the loop. I also converted the message to a error like the selector. Yesterday I failed to notice my linter complaining about score
being undefined. I was going to crack open some go documentation to learn how to do that right when I thought, this actually is more like an error anyway -- like the selector.
Anyway, hope this is helpful. If not, no worries. I appreciate your tool. When I was getting started in k8s, it was invaluable to avoiding potholes.
PS - I built locally and ran the tests wo/ failure
Thanks, this looks great!
I was troubleshooting a strange kube-score failure regarding PodDisruptionBudget.
kube-score was complaining that a deployment didn't have a PDB even though it very much did.
Going back to the code I realized the test was exiting early due to the namespace check:
https://github.com/zegl/kube-score/blob/0b3f154ca3f06a13323431a7d2199a74a1869fbc/score/disruptionbudget/disruptionbudget.go#L23-L25
Obviously, this was a bad manifest on my end (explicit is better than implicit and the tests passed as soon as I added
namespace: {{ .Release.Namespace }}
to my helm charts), but perhaps with a bit more output we could help other folks connect the dots faster.