55143dcad4
Merge pull request 'feat: add dry-run functionality and similar dependency' (#62) from feat/dryRun into master
That is inconvenient to use. Should be much shorter with something like :
I guess operator is not any string? Probably should be an enum too.
I think we already do have a Label type somewhere. I think it would be more appropriate here than String. That's true for all the label related fields in this file.
Not a String, should be a path type of some sort. I think we already handle this somewhere else correctly.
Yeah ok, I see what you did there.
But this is not a good use case for Macros. Macros are a last resort when the type semantics are already correct and we can't rely on the regular rust type…
OK that makes sense. Why not implement Display? I guess the format is not the expected one?
Also this file really deserves some tests to make sure the config output is correct.
What's the use case for this? I don't see this used in this pr.
Looks pretty good overall. Some minor refactoring comments but the rest is great!
For clarity, this is fine-ish to be in the kube_prometheus mod because it is specifically for pvc alerts but I think alert definitions should be in another module called just prometheus. kube_prometheus is for stuff specific to deploying prometheus on k8s. It is very possible to have a prometheus deployed somewhere else (AWS managed or grafana cloud maybe) which scrapes k8s targets and will want this alert.
Is this really Dell specific?
Nothing in this file seems Dell specific?