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

Add update info to serverinfo api call #434

Merged
merged 1 commit into from May 23, 2023
Merged

Conversation

peschmae
Copy link
Contributor

@peschmae peschmae commented Apr 9, 2023

Add information if nextcloud update is available to the SystemStatistics.
Will show the output of the last update check and not perform a new one.

If no new version is available update. available_version will be null

Fixes #211

@peschmae
Copy link
Contributor Author

peschmae commented May 3, 2023

Updated pull requests with fixes from php-cs fixer

@szaimen
Copy link
Contributor

szaimen commented May 11, 2023

@JuliaKirschenheuter you should be able to merge this already with your review...

@szaimen szaimen removed their request for review May 11, 2023 12:58
@JuliaKirschenheuter
Copy link
Collaborator

JuliaKirschenheuter commented May 12, 2023

@blizzz could you please make a second review?

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

json_decode invocation

lib/SystemStatistics.php Outdated Show resolved Hide resolved
@szaimen szaimen requested a review from blizzz May 16, 2023 09:51
@blizzz
Copy link
Member

blizzz commented May 19, 2023

And another thing, sorry i did not realize it earlier, json_decode() may return null when decoding fails, passing this directly to count will result in an PHP error already. To the decoding earlier (then it's also only needed once) and check for an array and existance of 'version' key, to have this logic robust.

@peschmae
Copy link
Contributor Author

Refactored logic into handling the lastupdateResult properly, and checking if it's an array or not.
If no new version is available the key available_version will be omitted

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.

Thank you for your pull request 👍

It would be better if the updatenotification app, which comes with the server, had an endpoint to ask for this information programmatically.

Fine by me to expose that an update is available. If we decide to keep lastupdatedat and available_version this should go into an own method (similar to getAppsInfo) to keep the code simple.

@@ -54,8 +54,20 @@ public function __construct(IConfig $config, IAppManager $appManager, Installer
public function getSystemStatistics(): array {
$processorUsage = $this->getProcessorUsage();
$memoryUsage = $this->os->getMemory();
$updateInfo = [];
$updateInfo['lastupdatedat'] = (int) $this->config->getAppValue('core', 'lastupdatedat');
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you need lastupdatedat for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lastupdatedat is useful to verify that the update check was run recently, and the information isn't totally outdated. I'm open to better ideas how to name it, since it might be unclear what was last updated, the server or the information about updates? (see comment below for indepth response)

if (is_array($lastUpdateResult)) {
$updateInfo['available'] = ( count($lastUpdateResult) > 0);
if( array_key_exists('version', $lastUpdateResult)) {
$updateInfo['available_version'] = $lastUpdateResult['version'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you need available_version for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the available_version exposed, let's us check directly from monitoring if the update is a minor or major upgrade, compared to the installed version. (see comment below for indepth response)

@peschmae
Copy link
Contributor Author

It would be better if the updatenotification app, which comes with the server, had an endpoint to ask for this information programmatically.

I agree that this might make more sense thematically, but since the available app updates are already exposed by the serverinfo this seems like a reasonable addition.
Currently the serverinfo endpoint is used for most monitoring purposes (eg. https://github.com/xperimental/nextcloud-exporter ), and having the server/core updates exposed to monitoring is very useful to us, as we can have a dashboard showing which instances have updates available.

Having the available_version exposed, let's us check directly from monitoring if the update is a minor or major upgrade, compared to the installed version.

lastupdatedat is useful to verify that the update check was run recently, and the information isn't totally outdated. I'm open to better ideas how to name it, since it might be unclear what was last updated, the server or the information about updates?

@kesselb
Copy link
Collaborator

kesselb commented May 23, 2023

To fix the linter and improve readability.

Index: lib/SystemStatistics.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/SystemStatistics.php b/lib/SystemStatistics.php
--- a/lib/SystemStatistics.php	(revision e0b02a805cb2c1bb3c118d96eb3274ddd6b82a34)
+++ b/lib/SystemStatistics.php	(date 1684832235249)
@@ -77,20 +77,21 @@
 
 	/**
 	 * Get info about server updates and last checked timestamp
-	 * 
+	 *
 	 * @return array information about core updates
 	 */
-	protected function getServerUpdateInfo(): array{
-		$updateInfo = [];
-		$updateInfo['lastupdatedat'] = (int) $this->config->getAppValue('core', 'lastupdatedat');
+	protected function getServerUpdateInfo(): array {
+		$updateInfo = [
+			'lastupdatedat' => (int) $this->config->getAppValue('core', 'lastupdatedat'),
+			'available' => false,
+		];
+
 		$lastUpdateResult = json_decode($this->config->getAppValue('core', 'lastupdateResult'), true);
 		if (is_array($lastUpdateResult)) {
 			$updateInfo['available'] = (count($lastUpdateResult) > 0);
 			if (array_key_exists('version', $lastUpdateResult)) {
 				$updateInfo['available_version'] = $lastUpdateResult['version'];
 			}
-		} else {
-			$updateInfo['available'] = false;
 		}
 
 		return $updateInfo;

To fix the DCO check, please sign off your commits: https://probot.github.io/apps/dco/

Signed-off-by: Mathias Petermann <mathias.petermann@gmail.com>
@peschmae
Copy link
Contributor Author

Applied patch, squashed commits, signed-off

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.

Add available Nextcloud update information
5 participants