Skip to content
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

Disks filtering by mount points from a configurable parameter #572

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

proxyconcept
Copy link

Here is my latest version from #426

Use a new system config variable serverinfo_disk_filter_paths that can contains a list of paths (actually a simple string with : as delimiter). If defined, only disks with a corresponding mount point are shown.

The paths list can contains special name (DOCROOT, DATADIR, TMPDIR and LOGSDIR) which are automatically converted (example: "serverinfo_disk_filter_paths" => "DATADIR:/var/log").

It works perfectly for my needs, I usually configure my Nextcloud instances with : occ config:system:set --value="DATADIR" -- serverinfo_disk_filter_paths

@proxyconcept proxyconcept marked this pull request as draft March 15, 2024 17:03
@proxyconcept proxyconcept marked this pull request as ready for review March 15, 2024 17:08
Use a new system config variable 'serverinfo_disk_filter_paths' that can contains a list of paths (actually a simple string with ':' as delimiter).
If defined, only disks with a corresponding mount point are shown.
The paths list can contains special name (DOCROOT, DATADIR, TMPDIR and LOGSDIR) which are automatically converted (example: "serverinfo_disk_filter_paths" => "DATADIR:/var/log").


Signed-off-by: Grégory Marigot <gmarigot@teicee.com>
Signed-off-by: Grégory Marigot <gmarigot@teicee.com>
Signed-off-by: Grégory Marigot <gmarigot@teicee.com>
Signed-off-by: Grégory Marigot <gmarigot@teicee.com>
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Copy link
Collaborator

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your pull request 👍

@@ -70,7 +72,47 @@ public function getUptime(): int {
}

public function getDiskInfo(): array {
return $this->backend->getDiskInfo();
$disks = $this->backend->getDiskInfo();
$filters = $this->config->getSystemValue('serverinfo_disk_filter_paths', null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two configuration systems in Nextcloud: SystemConfig and AppConfig

I'm using AppConfig here in serverinfo nowadays.

Suggested change
$filters = $this->config->getSystemValue('serverinfo_disk_filter_paths', null);
$filters = $this->config->getAppValue('disk_filter', '');

You can also use occ to configure it.
For example like https://github.com/nextcloud/serverinfo?tab=readme-ov-file#restricted-mode--nextcloud-28

return $this->backend->getDiskInfo();
$disks = $this->backend->getDiskInfo();
$filters = $this->config->getSystemValue('serverinfo_disk_filter_paths', null);
if ($filters === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($filters === null) {
if ($filters === '') {

If you follow my suggestion from above, don't forget to replace null with empty string.

return $disks;
}
// apply defined filters to restrict the list of disks according to their mount point
$filtered_disks = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$filtered_disks = [];
$filteredDisks = [];

CamelCase please ;)

// convert special filters to their corresponding paths
switch ($filter) {
case 'DOCROOT': $path = '';
if (isset($_SERVER['SCRIPT_FILENAME'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the docroot special filter because it might be empty.

A good alternative might be \OC::$SERVERROOT which always point to the nextcloud installation dir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants