Exclude profiles with labels from history. #4910
Conversation
blkt
force-pushed
the
feat/history-list-exclude-profiles-with-labels
branch
from
8adedfc
to
8edd02a
Compare
database/query/eval_history.sql
Outdated
| -- implicit filter, exclude all labelled profiles | ||
| AND p.labels = ARRAY[]::TEXT[] |
There was a problem hiding this comment.
Is it possible to request the history of a particular labelled profile, or is that currently verboten?
There was a problem hiding this comment.
It's a matter of choice, let me bring this up internally.
There was a problem hiding this comment.
I added the possibility to filter by labels.
blkt
force-pushed
the
feat/history-list-exclude-profiles-with-labels
branch
5 times, most recently
from
5d1e186
to
de835ad
Compare
History list showed evaluation results even for profiles having labels. This is different from the behaviour of Profile list and status, which by default only showed results for profiles without labels. This change makes the behaviour of History list closer to the one Profile list and status by adding a new filter to the `ListEvaluationHistory` endpoint. Both inclusion and exclusion are supported, and the additional `*` flag is allowed specifically for inclusion. Fixes stacklok/minder-stories#102
blkt
force-pushed
the
feat/history-list-exclude-profiles-with-labels
branch
from
de835ad
to
2cbfaa3
Compare
| // The default is to return all user-created profiles; the string "*" can | ||
| // be used to select all profiles, including system profiles. This syntax | ||
| // may be expanded in the future. | ||
| repeated string label_filter = 11 [ |
There was a problem hiding this comment.
Should this be a single string, similar to what the evaluation history and list profiles requests have?
There was a problem hiding this comment.
Filters usable in the history-list endpoint are all repeated string so that they can be sent (e.g. via a REST GET) as separate entries (e.g. ?label_filter=foo&label_filter=bar), as comma separated strings (e.g. ?label_filter=foo,bar), or as a combination of the two.
The handler is responsible for splitting the string and populating the struct input to the service/db layer as an array of strings. Layers outside of the control plane only reason in terms of array of strings.
My understanding of label_filter in Profiles is that it can be only passed as comma separated string, which is supported in this case as well. Additionally, the database layer is responsible for splitting the string, while here the responsibility is separate, which hopefully makes lower interfaces clearer.
TL;DR: it already support comma separated strings of labels, I would keep it this way.
| @@ -357,6 +374,29 @@ func (filter *listEvaluationFilter) ExcludedProfileNames() []string { | |||
| return filter.excludedProfileNames | |||
| } | |||
|
|
|||
| func (filter *listEvaluationFilter) AddLabel(label string) error { | |||
| if label == "!*" { | |||
There was a problem hiding this comment.
This looks like the same logic that exists in LabelsFromFilter, should it be centralized somewhere so it's not duplicated? https://github.com/mindersec/minder/blob/main/internal/db/domain.go#L49
There was a problem hiding this comment.
I agree that this logic should be shared, but history list RPC splits the responsibility between the controlplane and the service layer, while LabelsFromFilter wraps it in the database layer.
I would update how the LabelsFromFilter works by doing the same as done here, but in a separate PR. I can open a task for proper prioritization.
Summary
Filter by labels when listing history.
History list showed evaluation results even for profiles having labels. This is different from the behaviour of Profile list and status, which by default only showed results for profiles without labels.
This change makes the behaviour of History list closer to the one Profile list and status by adding a new filter to the
ListEvaluationHistoryendpoint. Both inclusion and exclusion are supported, and the additional*flag is allowed specifically for inclusion.Fixes stacklok/minder-stories#102
Change Type
Testing
Unit tested.
Review Checklist: