Re: [PATCH proxmox{,-backup} 00/14] fix #7904: Implement Enable checkboxes for all jobs

["Nicolas Frey" <n.frey@proxmox.com>]

Hi, thanks for the patches!

> The patches depend on each other in the way that the first
> patch of each "kind" introduces a new function or a new scheme
> which is then also used in later commits.
> If this is bad practice please give according feedback and i will
> split that out either in a v2 or in future patches.
>

overall I don't like the duplication of commit messages and would
prefer something more along the lines of (note that these are just
a suggestion, you don't have to name the commits this):

proxmox:
	pbs-api-types: jobs: add 'enable' flag to {sync,verify,tape,gc} job schema
	add 'gc_enable' flag to datastore config

proxmox-backup:
	// independent fixes should be put up-front so they can be applied even
	// if the rest of the series still needs work
	ui: prune job: change field label 'Enable' -> 'Enabled'
	// this is the commit that actually fixes the bug/adds the feature,
	// the other ones are either set up commits or add parity to the UI
	fix #7904: api: jobs: expose 'enable' option for {sync,verify,tape,gc} jobs
	// so this could also be prefixed with a fix #7904
	ui: job edit: add checkbox to enable {sync,verify,tape,gc} jobs
	ui: job status: add renderer for job's enabled flag
	ui: calendar event: remove clear button from calendar selector

for commit naming, we usually have some tags before the actual commit
subject, which can quite easily be determined by path, e.g.:

pbs-api-types/src/jobs.rs -> pbs-api-types: jobs: ...

if you're stuck on which tags to use, you can always look at past commit
subjects touching that file, e.g.:

```
git log --oneline pbs-api-types/src/jobs.rs
```

For the commits that contain the fix #xxxx tag, you should also consider
adding a Trailer for it:

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7904

###

Also, some shallow notes from glancing over the rest of the code:

The `next_run` logic could perhaps use a helper method to reduce code
duplication, and the way `next_run` is determind could be written a
bit more idiomatically (hint: use `cargo clippy`, which will tell you
some of this stuff!):

```
         let status = compute_schedule_status("tape-backup-job", &job.id, job.schedule.as_deref())?;

-        let next_run: i64;
-        let enabled = match job.enable {
-            Some(c) => c,
-            None => true,
-        };
-        if !enabled {
-            next_run = 0;
+        // default set to `true` to be backwards compatible
+        let next_run = if job.enable.unwrap_or(true) {
+            status.next_run.unwrap_or(current_time)
         } else {
-            next_run = status.next_run.unwrap_or(current_time);
-        }
+            0
+        };

         let mut next_media_label = None;
```