IETF Last Call Review of draft-ietf-netmod-schedule-yang-07
review-ietf-netmod-schedule-yang-07-opsdir-lc-andersson-2025-06-27-00

Reviewer: Per Andersson
Review Result: Has Nits

I have reviewed this document as part of the Operational directorate's
ongoing effort to review all IETF documents being processed by the IESG.
These comments were written with the intent of improving the operational
aspects of the IETF drafts. Comments that are not addressed in last call
may be included in AD reviews during the IESG review. Document editors
and WG chairs should treat these comments just like any other last call
comments.


Summary:

The document defines common types and groupings for scheduling purposes.
A YANG module is defined which includes a set of recurrence related
groupings with basic to advanced levels of representation. Groupings for
validating schedules and reporting schedule status are also defined.

I find the document easy to read and to understand, with a good
presentation of the proposed solution.


Major issues:

There are no major issues.


Minor issues:

* Section 3.3.6

  For readers unaware of the yang:timeticks type definition a reference
  to RFC 6991 for the period-timeticks using the yang:timeticks type
  could be good.


* Section 3.3.8

  Why is the iCalendar BYWEEKNO, BYMONTH, and WKST defined as
  "byyearweek", "byyearmonth", and "workweek-start" respectively?
  Suggest to follow the naming convention from RFC 5545, e.g.
  "byweekno", "bymonth", and "week-start".


* Section 3.3.9

  Why have a leaf with current local time instead of the pattern that
  was used before with yang:date-and-time leafs and a
  timezone-identifier if the times are in local time? This incurs
  different calculations to be made to infer the time offset from UTC
  for the schedule-status groupings and the other groupings. In one the
  timezone is supplied and can be feed to a time generating function to
  calculate the localtime, in the other the current UTC timestamp is
  diffed from the supplied localtime to calculate what timezone (or
  offset from UTC) is used.

  Suggest to use the pattern with timezone-identifier even for the
  schedule-status groupings.


* Section 6

  RFC 6991 is listed as a normative reference. However the numerical
  values in the must statement in period-start are magical constants in
  the YANG module without any further explanation. Suggest to add a
  reference and elaborate what the leaf values represent, instead of
  just stating "start/stop time".


Nits:

The following commands yield a diff

$ pyang -f yang --keep-comments --yang-line-length=69 \
        ietf-schedule@2025-05-30.yang  > new.yang
$ diff ietf-schedule@2025-05-30.yang new.yang

11d10
<
283a283
>
439,440c439
<         error-message
<           "Frequency must be provided.";
---
>         error-message "Frequency must be provided.";
594,611c593,609
<         must
<           "(not(derived-from(../../frequency,"
<          +"'schedule:secondly')) or (current() < 100)) and "
<          +"(not(derived-from(../../frequency,"
<          +"'schedule:minutely')) or (current() < 6000)) and "
<          +"(not(derived-from(../../frequency,'schedule:hourly'))"
<          +" or (current() < 360000)) and "
<          +"(not(derived-from(../../frequency,'schedule:daily'))"
<          +" or (current() < 8640000)) and "
<          +"(not(derived-from(../../frequency,'schedule:weekly'))"
<          +" or (current() < 60480000)) and "
<          +"(not(derived-from(../../frequency,"
<          +"'schedule:monthly')) or (current() < 267840000)) and "
<          +"(not(derived-from(../../frequency,'schedule:yearly'))"
<          +" or (current() < 3162240000))" {
<         error-message
<           "The period-start must not exceed the frequency
<            interval.";
---
>         must "(not(derived-from(../../frequency,"
>            + "'schedule:secondly')) or (current() < 100)) and "
>            + "(not(derived-from(../../frequency,"
>            + "'schedule:minutely')) or (current() < 6000)) and "
>            + "(not(derived-from(../../frequency,'schedule:hourly'))"
>            + " or (current() < 360000)) and "
>            + "(not(derived-from(../../frequency,'schedule:daily'))"
>            + " or (current() < 8640000)) and "
>            + "(not(derived-from(../../frequency,'schedule:weekly'))"
>            + " or (current() < 60480000)) and "
>            + "(not(derived-from(../../frequency,"
>            + "'schedule:monthly')) or (current() < 267840000)) and "
>            + "(not(derived-from(../../frequency,'schedule:yearly'))"
>            + " or (current() < 3162240000))" {
>           error-message
>             "The period-start must not exceed the frequency
>              interval.";
674,675c672,673
<           +  "(derived-from(../../frequency, 'schedule:yearly') "
<           +  " and not(../../byyearweek))";
---
>            + "(derived-from(../../frequency, 'schedule:yearly') "
>            + " and not(../../byyearweek))";


Spelling:

iclander -> iCalender
occurences -> occurrences
avaliable -> available
paramter -> parameter
prioritised -> prioritized
specifc -> specific
maxinum -> maximum
difficient -> difficult (?)
feaure -> feature
YANGDOCTORS -> YANG Doctors


Thanks for your contribution!


--
Per