Support matchers in rules API#1843
Conversation
Similar to the older change on Labels API (prometheus#828), this adds `matches` to the `Rules` API. This is a breaking change Signed-off-by: Joel Takvorian <jtakvori@redhat.com>
jotak
requested review from
ArthurSens,
bwplotka,
kakkoyun and
vesari
as code owners
| Snapshot(ctx context.Context, skipHead bool) (SnapshotResult, error) | ||
| // Rules returns a list of alerting and recording rules that are currently loaded. | ||
| Rules(ctx context.Context) (RulesResult, error) | ||
| Rules(ctx context.Context, matches []string) (RulesResult, error) |
There was a problem hiding this comment.
Not a blocker, how about making matches a variadic argument? It would make it neater
There was a problem hiding this comment.
I can do it if desired, but I also see some cons:
- it's less consistent with other API functions that also use
matches []string, such asLabelNames - if the signature has to change again later to add more arguments, depending on what those new arguments are, variadic may have to be reconsidered (e.g. if another
[]string-like argument is added)
I don't really have an opinion here, let me know what's best
There was a problem hiding this comment.
Also, I only proposed to add matches because it's the use-case I'm interested in, but there are a bunch of other arguments that could be relevant, according to the doc. I guess I'm trying to avoid a bigger refactoring, if that makes sense. But a more future-proof change would be to accept a parameters struct.
There was a problem hiding this comment.
Yes, never mind. I was lazy, and I hadn't checked the other parameters.
Even though we consider the API package as experimental, let's stick to the existing conventions.
We can add Options if needed.
When it comes to making it future-proof, maybe we can have the type as a first-order parameter as well and make the rest optional.
Any thoughts? cc @bwplotka
There was a problem hiding this comment.
I would vote for sticking to consistency and breaking ppl here, as we historically did on this experimental package. We should likely deprecate and move to exp or api module to make it clearer that it's experimental... (or make OpenAPI/swagger flow possible finally)
I would say LGTM for now
3 participants
Similar to the older change on Labels API (#828), this adds
matchesto theRulesAPI.This is a breaking change. If sending breaking changes is an issue, we could create another function signature - but that would be less consistent with the existing code base.
breaking change migration:
Compilation will fail for client callers who use
api.Rules(ctx).Simply change it to
api.Rules(ctx, nil)to keep the same behaviour.