Re: [pbs-devel] [PATCH v3 proxmox-backup 01/14] add two new schemas for verification jobs

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

On 13.10.20 11:32, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/types/mod.rs | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> index 75b68879..34468dc3 100644
> --- a/src/api2/types/mod.rs
> +++ b/src/api2/types/mod.rs
> @@ -302,7 +302,7 @@ pub const PRUNE_SCHEDULE_SCHEMA: Schema = StringSchema::new(
>      .format(&ApiStringFormat::VerifyFn(crate::tools::systemd::time::verify_calendar_event))
>      .schema();
>  
> -pub const VERIFY_SCHEDULE_SCHEMA: Schema = StringSchema::new(

this breaks build, VERIFY_SCHEDULE_SCHEMA is still used in config::datastore
and the respective API module, you only remove that over 4 (!) patches at the
end of the series.

No commit should ever break the build.
That is a no-go, hard to review and a PITA when doing a bisect.

Also, independent of that, I do not really get why this has to be it's
own patch - why not squash at least patch 01 to 03, they are effectively
one contained change, add a new verification job config with API plumbing.

Having patch 04 then separate can be ok, even wanted, it is not related
directly to the configuration, but patch 05 could be squashed into 04.

patch 06, to 10 is ok as is.

The whole removal should be done as single patch at the end.

I'd also at least add a snipped to drop the now unrecognized "verify-schedule"
field, while we do not care for backwards compat, we also do not want to break
all setups which had this set and force them to use the editor to drop that line.

There are at least two options:

1. a postinst script, similar to what we did for the termproxy change in the tasks
   active log, e.g.:
   # flock -w 30 /etc/proxmox-backup/.datastore.lck sed -i '/^\s\+verify-schedule /d' /etc/proxmox-backup/datastore.cfg

2. keeping a dummy "verify-schedule", with the schema dropped, which always gets deleted
   when writing out

I rather like 1. as it does not makes the real backup code uglier, and it's still
locked, plus we can use the fact that we can check the old version in postinst and
so do this edge triggered only.

> +pub const VERIFICATION_SCHEDULE_SCHEMA: Schema = StringSchema::new(
>      "Run verify job at specified schedule.")
>      .format(&ApiStringFormat::VerifyFn(crate::tools::systemd::time::verify_calendar_event))
>      .schema();
> @@ -324,6 +324,16 @@ pub const REMOVE_VANISHED_BACKUPS_SCHEMA: Schema = BooleanSchema::new(
>      .default(true)
>      .schema();
>  
> +pub const IGNORE_VERIFIED_BACKUPS_SCHEMA: Schema = BooleanSchema::new(
> +    "Do not verify backups that are already verified if their verification is not outdated.")
> +    .default(true)
> +    .schema();
> +
> +pub const VERIFICATION_OUTDATED_AFTER_SCHEMA: Schema = IntegerSchema::new(
> +    "Days after that a verification becomes outdated")
> +        .minimum(1)
> +        .schema();
> +
>  pub const SINGLE_LINE_COMMENT_SCHEMA: Schema = StringSchema::new("Comment (single line).")
>      .format(&SINGLE_LINE_COMMENT_FORMAT)
>      .schema();
>