-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enhancement: Add sorting option to the list policies method options #320
Conversation
Signed-off-by: serdar <serdar@cerbos.dev>
Signed-off-by: serdar <serdar@cerbos.dev>
Signed-off-by: serdar <serdar@cerbos.dev>
Signed-off-by: serdar <serdar@cerbos.dev>
client/model.go
Outdated
field: field, | ||
} | ||
// Sort enables sorting the policies by with given field. | ||
func Sort(field ListPoliciesSortingType, descending ...bool) ListOpt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a little bit controversial but fits nicely as an optional field. Let me know if we should make this a mandatory argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid boolean arguments in public methods. I think there should be two ListOpt
s here called SortAscending
and SortDescending
.
client/model.go
Outdated
field: field, | ||
} | ||
// Sort enables sorting the policies by with given field. | ||
func Sort(field ListPoliciesSortingType, descending ...bool) ListOpt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid boolean arguments in public methods. I think there should be two ListOpt
s here called SortAscending
and SortDescending
.
internal/svc/list.go
Outdated
func (s nameOrder) Swap(i, j int) { s[i], s[j] = s[j], s[i] } | ||
|
||
func (s nameOrder) Less(i, j int) bool { | ||
return getPolicyName(s[i]) < getPolicyName(s[j]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the policy names are the same, then compare version to determine which comes first.
internal/svc/list.go
Outdated
func (s versionOrder) Swap(i, j int) { s[i], s[j] = s[j], s[i] } | ||
|
||
func (s versionOrder) Less(i, j int) bool { | ||
return getPolicyVersion(s[i]) < getPolicyVersion(s[j]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. When versions are the same, compare names.
Signed-off-by: serdar <serdar@cerbos.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Added sorting option the the ListPolicies method. I had to rename the
FilterOpt
toListOpt
in the Go client so that we can use sort option together with the filter options. And we don't need to break the method signature published withv.0.6
. Also did some minor file organization (ie. addedlist.go
insvc
package forListPolicies
related code)As per discussions done offline, we decided to define an Enum for sortable fields instead of allowing users to use JSONPath expressions.
Two flags added to the
cerbosctl
and--sort name
is set by default:The Go client can accept two new options as sorting option:
The sorting is done in the server so that it can work better with pagination and also we can proxy it to the DB.
Fixes #298
Checklist
git commit -s ...
) to provide the DCO