Re: [pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 22/09/2023 um 11:13 schrieb Stefan Lendl:
> Handling of GC tasks that never ran works great now. GUI alignment looks good.

Thanks for testing, but two general things:

- use reply-all and ensure your mail user agent is not borked so that it
  includes the in-reply-to header important for making mail threads
  work.

- trim the context in your replies to important stuff. E.g., the mail
  I'm replying to has about 1060 lines, of which a few dozens are
  relevant at most.

- can you post a "Tested-by: Full Name <email>" trailer, I try to amend
  them to the commit message and thus keep track of such information (if
  somebody in the future has questions about this, and the author is
  then currently not available, they could ask you if you got some idea
  about whatever their question is)


> Am 20/09/2023 um 19:02 schrieb Gabriel Goller:
>> Added `ObjectGrid` for GCView, moved to different file. Added
>> endpoint `gc_info` that returns all the necessary config and
>> last run stats.
> 

Sorry to only notice this on v3, but can you please split this patch
into:
- new API
- moving GC view out to own file (with as little semantic change as
  possible)
- adding new UI

In general, it can be fine to do API and UI in one patch if they are
quite coupled, and it's a relatively small and targeted feature. But, if
there's existing UI code it still can be nicer to split this so one can
differ easier between file-movement changes and actual code changes
(git's color-moved isn't IME that helpful)


[...]

>> +    // calculate next event
>> +    if let Some(schedule) = &store_config.gc_schedule {
>> +        if let (Ok(event), Some(last_run)) = (
>> +            schedule.parse::<CalendarEvent>(),
>> +            computed_schedule.last_run_endtime,
>> +        ) {
>> +            if let Ok(next_event) = event.compute_next_event(last_run) {
>> +                computed_schedule.next_run = next_event;
>> +            }
>> +        }
>> +    }
> 
> 
> If GC never ran but is scheduled, it would also be nice to show the next
> scheduled run.
> This might be usefull if people had not GC configured, then create a job
> to further indicate that the job is scheduled correctly.
> 
> 

I did not check it out closely, but how is the situation handled if it's
currently running, returning that under "last_run_upid" would be
probably confusing, but without >UPID one cannot allow one to open the
log easily, or do we want to have this strictly for last- and next-run?