From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject:
 Re: [PATCH 7/9] pakfire: Refactor status function separating UI and logic
Date: Mon, 21 Mar 2022 16:28:21 +0000
Message-ID: <69EC2562-4648-495F-8F4A-BD7E9975053E@ipfire.org>
In-Reply-To: <20220309225655.4472-8-robin.roevens@disroot.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6823163918137276824=="
List-Id: <development.lists.ipfire.org>

--===============6823163918137276824==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hey,

> On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens(a)disroot.org> wrote:
>=20
> - removed UI related code from status function and refactor it
>  to returning hash representing pakfire status.
> - Add UI part to pakfire script
> - Use pakfire status function in pakfire.cgi, removing duplicate code
>=20
> Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> ---
> html/cgi-bin/pakfire.cgi     | 39 ++++++++++++++++-------------------
> src/pakfire/lib/functions.pl | 40 ++++++++++++++++++------------------
> src/pakfire/pakfire          | 13 +++++++++++-
> 3 files changed, 50 insertions(+), 42 deletions(-)
>=20
> diff --git a/html/cgi-bin/pakfire.cgi b/html/cgi-bin/pakfire.cgi
> index 30a23128b..d62b6058f 100644
> --- a/html/cgi-bin/pakfire.cgi
> +++ b/html/cgi-bin/pakfire.cgi
> @@ -37,6 +37,9 @@ my %color =3D ();
> my %pakfiresettings =3D ();
> my %mainsettings =3D ();
>=20
> +# Get Pakfire status
> +my %pakfire_status =3D &Pakfire::status();
> +
> # Load general settings
> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
> &General::readhash("/srv/web/ipfire/html/themes/ipfire/include/colors.txt",=
 \%color);
> @@ -66,7 +69,7 @@ if($cgiparams{'ACTION'} eq 'json-getstatus') {
> 	my %status =3D (
> 		'running' =3D> &_is_pakfire_busy() || "0",
> 		'running_since' =3D> &General::age("$Pakfire::lockfile") || "0s",
> -		'reboot' =3D> (-e "/var/run/need_reboot") || "0",
> +		'reboot' =3D> ("$pakfire_status{'RebootRequired'}" eq "yes") || "0",
> 		'failure' =3D> $failure || "0"
> 	);

Okay.

> @@ -333,32 +336,26 @@ END
> 	exit;
> }
>=20
> -my $core_release =3D `cat /opt/pakfire/db/core/mine 2>/dev/null`;
> -chomp($core_release);
> -my $core_update_age =3D &General::age("/opt/pakfire/db/core/mine");
> -my $corelist_update_age =3D &General::age("/opt/pakfire/db/lists/core-list=
.db");
> -my $server_update_age =3D &General::age("/opt/pakfire/db/lists/server-list=
.db");
> -my $packages_update_age =3D &General::age("/opt/pakfire/db/lists/packages_=
list.db");
> -
> &Header::openbox("100%", "center", "Pakfire");
>=20
> print <<END;
> 	<table id=3D"pfmain">
> END
> -if ( -e "/var/run/need_reboot") {
> +if ("$pakfire_status{'RebootRequired'}" eq "yes") {
> 	print "\t\t<tr><td colspan=3D'2'><a href=3D'/cgi-bin/shutdown.cgi'>$Lang::=
tr{'needreboot'}!</a></td></tr>\n";
> }
> +
> print <<END;
> 		<tr><td class=3D"heading">$Lang::tr{'pakfire system state'}:</td>
> 			<td class=3D"heading">$Lang::tr{'available updates'}:</td></tr>
>=20
> -		<tr><td><strong>$Lang::tr{'pakfire core update level'}: $core_release</s=
trong>
> +		<tr><td><strong>$Lang::tr{'pakfire core update level'}: $pakfire_status{=
'Release'}</strong>
> 				<hr>
> 				<div class=3D"pflist">
> -					$Lang::tr{'pakfire last update'} $core_update_age $Lang::tr{'pakfire =
ago'}<br>
> -					$Lang::tr{'pakfire last serverlist update'} $server_update_age $Lang:=
:tr{'pakfire ago'}<br>
> -					$Lang::tr{'pakfire last core list update'} $corelist_update_age $Lang=
::tr{'pakfire ago'}<br>
> -					$Lang::tr{'pakfire last package update'} $packages_update_age $Lang::=
tr{'pakfire ago'}
> +					$Lang::tr{'pakfire last update'} $pakfire_status{'LastUpdate'} $Lang:=
:tr{'pakfire ago'}<br>
> +					$Lang::tr{'pakfire last serverlist update'} $pakfire_status{'LastServ=
erListUpdate'} $Lang::tr{'pakfire ago'}<br>
> +					$Lang::tr{'pakfire last core list update'} $pakfire_status{'LastCoreL=
istUpdate'} $Lang::tr{'pakfire ago'}<br>
> +					$Lang::tr{'pakfire last package update'} $pakfire_status{'LastPakList=
Update'} $Lang::tr{'pakfire ago'}
> 				</div>

Is it actually useful to show those timestamps? If anything, the last update =
of the lists (all together) would be interesting.

I do not think that it is important to know when the last update was installe=
d. How do you feel about this?

> 				<form method=3D'post' action=3D'$ENV{'SCRIPT_NAME'}'>
> 					<input type=3D'hidden' name=3D'ACTION' value=3D'update' />
> @@ -370,17 +367,17 @@ print <<END;
> 					<select name=3D"UPDPAKS" class=3D"pflist" size=3D"5" disabled>
> END
>=20
> -	my %coredb =3D &Pakfire::coredbinfo();
> -	if (defined $coredb{'AvailableRelease'}) {
> -		print "<option value=3D\"core\">Core-Update -- $coredb{'CoreVersion'} --=
 Release: $coredb{'Release'} -> $coredb{'AvailableRelease'}</option>\n";
> +	if ("$pakfire_status{'CoreUpdateAvailable'}" eq "yes") {
> +		print "<option value=3D\"core\">Core-Update -- $pakfire_status{'CoreVers=
ion'} -- Release: $pakfire_status{'Release'} -> $pakfire_status{'AvailableRel=
ease'}</option>\n";
> 	}

See my remarks on localisation in my other email.

>=20
> -	my %upgradelist =3D &Pakfire::dblist("upgrade");
> -	foreach my $pak (sort keys %upgradelist) {
> -		print "<option value=3D\"$pak\">Update: $pak -- Version: $upgradelist{$p=
ak}{'ProgVersion'} -> $upgradelist{$pak}{'AvailableProgVersion'} -- Release: =
$upgradelist{$pak}{'Release'} -> $upgradelist{$pak}{'AvailableRelease'}</opti=
on>\n";
> +	if ($pakfire_status{'PakUpdatesAvailable'} > 0) {
> +		my %upgradelist =3D &Pakfire::dblist("upgrade");
> +		foreach my $pak (sort keys %upgradelist) {
> +			print "<option value=3D\"$pak\">Update: $pak -- Version: $upgradelist{$=
pak}{'ProgVersion'} -> $upgradelist{$pak}{'AvailableProgVersion'} -- Release:=
 $upgradelist{$pak}{'Release'} -> $upgradelist{$pak}{'AvailableRelease'}</opt=
ion>\n";
> +		}
> 	}
>=20
> -
> 	print <<END;
> 					</select>
> 					<input type=3D'hidden' name=3D'ACTION' value=3D'upgrade' />
> diff --git a/src/pakfire/lib/functions.pl b/src/pakfire/lib/functions.pl
> index b35aed6a3..028a0277e 100644
> --- a/src/pakfire/lib/functions.pl
> +++ b/src/pakfire/lib/functions.pl
> @@ -897,26 +897,26 @@ sub reboot_required {
> }
>=20
> sub status {
> -	# General info
> -	my $return =3D "Core-Version: $Conf::version\n";
> -	$return .=3D "Core-Update-Level: $Conf::core_mine\n";
> -	$return .=3D "Last update: " . &General::age("/opt/pakfire/db/core/mine")=
 . " ago\n";
> -	$return .=3D "Last core-list update: " . &General::age("/opt/pakfire/db/l=
ists/core-list.db") . " ago\n";
> -	$return .=3D "Last server-list update: " . &General::age("/opt/pakfire/db=
/lists/server-list.db") . " ago\n";
> -	$return .=3D "Last packages-list update: " . &General::age("/opt/pakfire/=
db/lists/packages_list.db") . " ago\n";
> -
> -	# Get availability of core updates
> -	$return .=3D "Core-Update available: " . &Pakfire::coreupdate_available()=
 . "\n";
> -
> -	# Get availability of package updates
> -	$return .=3D "Package-Updates available: " . &Pakfire::updates_available(=
) . "\n";
> -
> -	# Test if reboot is required
> -	$return .=3D "Reboot required: " . &Pakfire::reboot_required() . "\n";
> -
> -	# Return status text
> -	print "$return";
> -	exit 1;
> +	### This subroutine returns pakfire status information in a hash.
> +	# Usage is without arguments
> +
> +	# Add core version info
> +	my %status =3D &Pakfire::coredbinfo();
> +
> +	# Add last update info
> +	$status{'LastUpdate'} =3D &General::age("/opt/pakfire/db/core/mine");
> +	$status{'LastCoreListUpdate'} =3D &General::age("/opt/pakfire/db/lists/co=
re-list.db");
> +	$status{'LastServerListUpdate'} =3D &General::age("/opt/pakfire/db/lists/=
server-list.db");
> +	$status{'LastPakListUpdate'} =3D &General::age("/opt/pakfire/db/lists/pac=
kages_list.db");
> +
> +	# Add number of available package updates
> +	$status{'CoreUpdateAvailable'} =3D (defined $status{'AvailableRelease'}) =
? "yes" : "no";
> +	$status{'PakUpdatesAvailable'} =3D &Pakfire::updates_available();
> +
> +	# Add if reboot is required
> +	$status{'RebootRequired'} =3D &Pakfire::reboot_required();
> +
> +	return %status;
> }
>=20
> sub get_arch() {
> diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
> index b529db77a..0ed8aacd4 100644
> --- a/src/pakfire/pakfire
> +++ b/src/pakfire/pakfire
> @@ -406,7 +406,18 @@
> 			system("rm -f /etc/fcron.daily/pakfire-upgrade");
> 		}
> 	} elsif ("$ARGV[0]" eq "status") {
> -		&Pakfire::status;
> +		my %status =3D &Pakfire::status;
> +
> +		print "Core-Version: $status{'CoreVersion'}\n";
> +		print "Core-Update-Level: $status{'Release'}\n";
> +		print "Last update: $status{'LastUpdate'} ago\n";
> +		print "Last core-list update: $status{'LastCoreListUpdate'} ago\n";
> +		print "Last server-list update: $status{'LastServerListUpdate'} ago\n";
> +		print "Last packages-list update: $status{'LastPakListUpdate'} ago\n";
> +		print "Core-Update available: $status{'CoreUpdateAvailable'}";
> +		print " ($status{'AvailableRelease'})" if ("$status{'CoreUpdateAvailable=
'}" eq "yes");
> +		print "\nPackage-Updates available: $status{'PakUpdatesAvailable'}\n";
> +		print "Reboot required: $status{'RebootRequired'}\n";

The return status was lost here. I do not know why we would always return 1, =
but I would say that it makes sense to indicate whether updates are available=
 or all is good.

> 	} else {
> 		&Pakfire::usage;
> 	}
> --=20
> 2.34.1
>=20
>=20
> --=20
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>=20


--===============6823163918137276824==--