Re: [PATCH 7/9] pakfire: Refactor status function separating UI and logic

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 10313 bytes --]

Hello,

> On 23 Mar 2022, at 19:56, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> Hi
> 
> Michael Tremer schreef op ma 21-03-2022 om 16:28 [+0000]:
>> Hey,
>> 
>>> On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens(a)disroot.org>
>>> wrote:
>>> 
>>> - 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
>>> 
>>> 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(-)
>>> 
>>> 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 = ();
>>> my %pakfiresettings = ();
>>> my %mainsettings = ();
>>> 
>>> +# Get Pakfire status
>>> +my %pakfire_status = &Pakfire::status();
>>> +
>>> # Load general settings
>>> &General::readhash("${General::swroot}/main/settings",
>>> \%mainsettings);
>>> &General::readhash("/srv/web/ipfire/html/themes/ipfire/include/colo
>>> rs.txt", \%color);
>>> @@ -66,7 +69,7 @@ if($cgiparams{'ACTION'} eq 'json-getstatus') {
>>> my %status = (
>>> 'running' => &_is_pakfire_busy() || "0",
>>> 'running_since' =>
>>> &General::age("$Pakfire::lockfile") || "0s",
>>> -  'reboot' => (-e "/var/run/need_reboot") || "0",
>>> +  'reboot' => ("$pakfire_status{'RebootRequired'}" eq
>>> "yes") || "0",
>>> 'failure' => $failure || "0"
>>> );
>> 
>> Okay.
>> 
>>> @@ -333,32 +336,26 @@ END
>>> exit;
>>> }
>>> 
>>> -my $core_release = `cat /opt/pakfire/db/core/mine 2>/dev/null`;
>>> -chomp($core_release);
>>> -my $core_update_age = &General::age("/opt/pakfire/db/core/mine");
>>> -my $corelist_update_age =
>>> &General::age("/opt/pakfire/db/lists/core-list.db");
>>> -my $server_update_age =
>>> &General::age("/opt/pakfire/db/lists/server-list.db");
>>> -my $packages_update_age =
>>> &General::age("/opt/pakfire/db/lists/packages_list.db");
>>> -
>>> &Header::openbox("100%", "center", "Pakfire");
>>> 
>>> print <<END;
>>> <table id="pfmain">
>>> END
>>> -if ( -e "/var/run/need_reboot") {
>>> +if ("$pakfire_status{'RebootRequired'}" eq "yes") {
>>> print "\t\t<tr><td colspan='2'><a href='/cgi-
>>> bin/shutdown.cgi'>$Lang::tr{'needreboot'}!</a></td></tr>\n";
>>> }
>>> +
>>> print <<END;
>>> <tr><td class="heading">$Lang::tr{'pakfire system
>>> state'}:</td>
>>> <td class="heading">$Lang::tr{'available
>>> updates'}:</td></tr>
>>> 
>>> -  <tr><td><strong>$Lang::tr{'pakfire core update
>>> level'}: $core_release</strong>
>>> +  <tr><td><strong>$Lang::tr{'pakfire core update
>>> level'}: $pakfire_status{'Release'}</strong>
>>> <hr>
>>> <div class="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{'LastServerListUpdate'}
>>> $Lang::tr{'pakfire ago'}<br>
>>> +  $Lang::tr{'pakfire last
>>> core list update'} $pakfire_status{'LastCoreListUpdate'}
>>> $Lang::tr{'pakfire ago'}<br>
>>> +  $Lang::tr{'pakfire last
>>> package update'} $pakfire_status{'LastPakListUpdate'}
>>> $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
>> installed. How do you feel about this?
> 
> I gave the current pakfire wui a good long stare.. and indeed I have to
> conclude that there is not much added value to show these.
> Last lists update is indeed the only timestamp that is interesting as
> there is also a button to refresh them now.
> But as I already plan to overhaul that page completely, I will
> integrate the removal of all those timestamps there.

Let’s talk about that plan before you start coding.

>> 
>>> <form method='post'
>>> action='$ENV{'SCRIPT_NAME'}'>
>>> <input type='hidden'
>>> name='ACTION' value='update' />
>>> @@ -370,17 +367,17 @@ print <<END;
>>> <select name="UPDPAKS"
>>> class="pflist" size="5" disabled>
>>> END
>>> 
>>> -  my %coredb = &Pakfire::coredbinfo();
>>> -  if (defined $coredb{'AvailableRelease'}) {
>>> -  print "<option value=\"core\">Core-Update --
>>> $coredb{'CoreVersion'} -- Release: $coredb{'Release'} ->
>>> $coredb{'AvailableRelease'}</option>\n";
>>> +  if ("$pakfire_status{'CoreUpdateAvailable'}" eq "yes") {
>>> +  print "<option value=\"core\">Core-Update --
>>> $pakfire_status{'CoreVersion'} -- Release:
>>> $pakfire_status{'Release'} ->
>>> $pakfire_status{'AvailableRelease'}</option>\n";
>>> }
>> 
>> See my remarks on localisation in my other email.
> Noted, will change it.
> 
>> 
>>> 
>>> -  my %upgradelist = &Pakfire::dblist("upgrade");
>>> -  foreach my $pak (sort keys %upgradelist) {
>>> -  print "<option value=\"$pak\">Update: $pak --
>>> Version: $upgradelist{$pak}{'ProgVersion'} ->
>>> $upgradelist{$pak}{'AvailableProgVersion'} -- Release:
>>> $upgradelist{$pak}{'Release'} ->
>>> $upgradelist{$pak}{'AvailableRelease'}</option>\n";
>>> +  if ($pakfire_status{'PakUpdatesAvailable'} > 0) {
>>> +  my %upgradelist = &Pakfire::dblist("upgrade");
>>> +  foreach my $pak (sort keys %upgradelist) {
>>> +  print "<option value=\"$pak\">Update: $pak
>>> -- Version: $upgradelist{$pak}{'ProgVersion'} ->
>>> $upgradelist{$pak}{'AvailableProgVersion'} -- Release:
>>> $upgradelist{$pak}{'Release'} ->
>>> $upgradelist{$pak}{'AvailableRelease'}</option>\n";
>>> +  }
>>> }
>>> 
>>> -
>>> print <<END;
>>> </select>
>>> <input type='hidden'
>>> name='ACTION' value='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 {
>>> }
>>> 
>>> sub status {
>>> -  # General info
>>> -  my $return = "Core-Version: $Conf::version\n";
>>> -  $return .= "Core-Update-Level: $Conf::core_mine\n";
>>> -  $return .= "Last update: " .
>>> &General::age("/opt/pakfire/db/core/mine") . " ago\n";
>>> -  $return .= "Last core-list update: " .
>>> &General::age("/opt/pakfire/db/lists/core-list.db") . " ago\n";
>>> -  $return .= "Last server-list update: " .
>>> &General::age("/opt/pakfire/db/lists/server-list.db") . " ago\n";
>>> -  $return .= "Last packages-list update: " .
>>> &General::age("/opt/pakfire/db/lists/packages_list.db") . " ago\n";
>>> -
>>> -  # Get availability of core updates
>>> -  $return .= "Core-Update available: " .
>>> &Pakfire::coreupdate_available() . "\n";
>>> -
>>> -  # Get availability of package updates
>>> -  $return .= "Package-Updates available: " .
>>> &Pakfire::updates_available() . "\n";
>>> -
>>> -  # Test if reboot is required
>>> -  $return .= "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 = &Pakfire::coredbinfo();
>>> +
>>> +  # Add last update info
>>> +  $status{'LastUpdate'} =
>>> &General::age("/opt/pakfire/db/core/mine");
>>> +  $status{'LastCoreListUpdate'} =
>>> &General::age("/opt/pakfire/db/lists/core-list.db");
>>> +  $status{'LastServerListUpdate'} =
>>> &General::age("/opt/pakfire/db/lists/server-list.db");
>>> +  $status{'LastPakListUpdate'} =
>>> &General::age("/opt/pakfire/db/lists/packages_list.db");
>>> +
>>> +  # Add number of available package updates
>>> +  $status{'CoreUpdateAvailable'} = (defined
>>> $status{'AvailableRelease'}) ? "yes" : "no";
>>> +  $status{'PakUpdatesAvailable'} =
>>> &Pakfire::updates_available();
>>> +
>>> +  # Add if reboot is required
>>> +  $status{'RebootRequired'} = &Pakfire::reboot_required();
>>> +
>>> +  return %status;
>>> }
>>> 
>>> 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 = &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.
> 
> Indeed, I missed the return value.. Will add one.
> 
> 
> Regards
> 
> Robin
>> 
>>> } else {
>>> &Pakfire::usage;
>>> }
>>> -- 
>>> 2.34.1
>>> 
>>> 
>>> -- 
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>> 
>> 
>> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.