Re: [pdm-devel] [PATCH datacenter-manager v3 00/11] backend implementation for view filters

["Michael Köppl" <m.koeppl@proxmox.com>]

Gave this a closer look. Apart from my comments on the individual
patches, the changes lgtm! Thanks for also including a rather extensive
test suite.

With my comments on the individual patches addressed, consider this:

Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>

On Thu Nov 6, 2025 at 2:43 PM CET, Lukas Wagner wrote:
> Key aspects:
>   - new config file at /etc/proxmox-datacenter-manager/views.cfg
>   - ViewConfig as a definition type, has
>      - {include,exclude} {remote,resource-id,resource-type,resource-pool,tag}:{value}
>
>   - View resource filter implementation & big suite of unit tests
>     - excludes are processed after includes
>     - if no include rules are defined, all resources but those which were
>       excluded are matched
>     - if no rules are defined in a filter, everything is matched
>
>   - Added the 'view' parameter to a couple of API endpoints
>     - /resources/list
>     - /resources/status
>     - /resources/subscription
>     - /resources/top-entities
>     - /remote-tasks/list
>     - /remote-tasks/statistis
>
>   - ACL checks are done on /view/{view-filter-id} for now, replace
>     any other permission check in the handler iff the view-filter paramter
>     is set
>
> Left to do:
>   - CRUD for filter definition
>   - UI for filter rules
>
> Changes since v2 (thx for the review @Dominik):
>
> - Renamed:
>   - ViewFilter -> View
>   - ViewFilterConfig -> ViewConfig
>   - 'view-filter' parameters to 'view'
>
> - Use ApiSectionDataEntry trait for ViewConfig
> - Use SAFE_ID_FORMAT to validate filter values
> - Changed path for config file from `views/filter.cfg` to just `views.cfg`
>
> - Include failed remotes in API responses iff they have been explicitly included
> via `include remote:<...>`. Previously, they had been filtered out to avoid
> leaking the existence of remotes, but if they have been explicitly included,
> this is fine.
>
> - Simplify `check_rules` function by using `.any()` instead of looping manually
>
> - Merged the rule implementation and test commits
>
> - Added views::get_optional_view as a convenience helper. This one is similar to
> views::get_view, but accepts Option<&str> and returns Option<View>.
>
> Changes since v1 (RFC):
>   - Change config key structure, moving the type into the value
>     e.g. 
>       include-remote foo
>     became
>       include remote:foo
>
>   - Minor fixes from the review (thanks Wolfgang & Shannon)
>
>
> proxmox-datacenter-manager:
>
> Lukas Wagner (11):
>   pdm-api-types: views: add ViewConfig type
>   pdm-config: views: add support for views
>   acl: add '/view' and '/view/{view-id}' as allowed ACL paths
>   views: add implementation for view resource filtering
>   api: resources: list: add support for view parameter
>   api: resources: top entities: add support for view parameter
>   api: resources: status: add support for view parameter
>   api: subscription status: add support for view parameter
>   api: remote-tasks: add support for view parameter
>   pdm-client: resource list: add view-filter parameter
>   pdm-client: top entities: add view-filter parameter
>
>  cli/client/src/resources.rs                  |   2 +-
>  lib/pdm-api-types/src/lib.rs                 |   8 +
>  lib/pdm-api-types/src/views.rs               | 202 ++++++
>  lib/pdm-client/src/lib.rs                    |  19 +-
>  lib/pdm-config/src/lib.rs                    |   2 +-
>  lib/pdm-config/src/views.rs                  |  17 +
>  server/src/acl.rs                            |   6 +
>  server/src/api/remote_tasks.rs               |  36 +-
>  server/src/api/resources.rs                  | 166 ++++-
>  server/src/lib.rs                            |   1 +
>  server/src/metric_collection/top_entities.rs |   5 +
>  server/src/remote_tasks/mod.rs               |  37 +-
>  server/src/resource_cache.rs                 |   3 +-
>  server/src/views/mod.rs                      | 205 ++++++
>  server/src/views/tests.rs                    | 619 +++++++++++++++++++
>  ui/src/dashboard/view.rs                     |   2 +-
>  ui/src/sdn/zone_tree.rs                      |   2 +-
>  17 files changed, 1283 insertions(+), 49 deletions(-)
>  create mode 100644 lib/pdm-api-types/src/views.rs
>  create mode 100644 lib/pdm-config/src/views.rs
>  create mode 100644 server/src/views/mod.rs
>  create mode 100644 server/src/views/tests.rs
>
>
> Summary over all repositories:
>   17 files changed, 1283 insertions(+), 49 deletions(-)



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel