[Freeipa-devel] [PATCH 0137] ipalib: Add DateTime parameter

[Tomas Babej]

On 01/14/2014 10:19 AM, Petr Viktorin wrote:
> On 01/14/2014 09:27 AM, Jan Cholasta wrote:
>> On 13.1.2014 14:57, Petr Vobornik wrote:
>>> On 13.1.2014 13:41, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 10.1.2014 21:21, Nathaniel McCallum wrote:
>>>>> On Thu, 2014-01-09 at 16:30 +0100, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Adds a parameter that represents a DateTime format using
>>>>>> datetime.datetime
>>>>>> object from python's native datetime library.
>>>>>>
>>>>>> In the CLI, accepts one of the following formats:
>>>>>> Accepts subset of values defined by ISO 8601:
>>>>>> %Y-%m-%dT%H:%M:%S
>>>>>> %Y-%m-%dT%H:%M
>>>>>> '%Y%m%dT%H:%M:%S'
>>>>>> '%Y%m%dT%H:%M'
>>>>>>
>>>>>> Also accepts LDAP Generalized time in the following format:
>>>>>> '%Y%m%d%H%M%SZ'
>>>>>>
>>>>>> As a simplification, it does not deal with timezone info and ISO
>>>>>> 8601
>>>>>> values with timezone info (+-hhmm) are rejected. Values are expected
>>>>>> to be in the UTC timezone.
>>>>>>
>>>>>> Values are saved to LDAP as LDAP Generalized time values in the
>>>>>> format
>>>>>> '%Y%m%d%H%SZ' (no time fractions and UTC timezone is assumed). To
>>>>>> avoid
>>>>>> confusion, in addition to subset of ISO 8601 values, the LDAP
>>>>>> generalized
>>>>>> time in the format '%Y%m%d%H%M%SZ' is also accepted as an input (as
>>>>>> this
>>>>>> is the
>>>>>> format user will see on the output).
>>>>>>
>>>>>> Part of: https://fedorahosted.org/freeipa/ticket/3306
>>>>>
>>>>> The date/time syntax formats are not compliant with ISO 8601. You
>>>>> stated
>>>>> they are expected to be in UTC timezone, but no 'Z' is expected in
>>>>> most
>>>>> of them. This is not only non-standard, but would prevent you from
>>>>> every
>>>>> supporting local time in the future.
>>>>
>>>> +1, please require the trailing "Z".
>>>>
>>>>
>>>> I think generalized time format should be the preferred one, as it is
>>>> stored in LDAP and thus returned by commands. Also, do we really need
>>>> all of these other formats?
>>>
>>> +1 That API should accept and always return generalized time.
>>>
>>> But UIs(CLI, Web) should display something more human readable. Like:
>>> %Y-%m-%dT%H:%M:%SZ or IMHO better (but not ISO 8601): %Y-%m-%d
>>> %H:%M:%SZ
>>> ie. 2014-03-08 14:16:55Z compared to 2014-03-08T14:16:55Z or
>>> 20140308141655Z
>>>
>>> Both should accept input value in the same format. Usage of different
>>> output and input formats is confusing. Thus I agree with supporting
>>> more
>>> input formats. Decision whether it should be done on API level or
>>> client
>>> level is another matter.
>>
>> If you want human readable output, you have to do the conversion from
>> generalized time on clients. If you want to support local time and
>> timezones, you have to do some processing on the client as well. I would
>> be perfectly happy if we supported only generalized time over the wire
>> and did conversion from/to human readable on clients.
>>
>>>
>>> I has been implementing the Web UI part and I think we should also
>>> support a formats without time component. My initial impl. supports
>>> combinations of:
>>>
>>>      var dates = [
>>>          ['YYYY-MM-DD', /^(\d{4})-(\d{2})-(\d{2})$/],
>>>          ['YYYYMMDD',/^(\d{4})(\d{2})(\d{2})$/]
>>>      ];
>>>
>>>      var times = [
>>>          ['HH:mm:ss', /^(\d\d):(\d\d):(\d\d)Z$/],
>>>          ['HH:mm', /^(\d\d):(\d\d)Z$/],
>>>          ['HH', /^(\d\d)Z$/]
>>>      ];
>>> + generalized time
>>> ['YYYYMMDDHHmmssZ',/^(\d{4})(\d{2})(\d{2})(\d{2})(\d{2})(\d{2})Z$/]
>>>
>>> with /( |T)/ as divider and time optional.
>>>
>>> Both UIs should also tell the user what is the expected format (at
>>> least
>>> one of them). Getting incorrect format error without knowing it is
>>> annoying.
>>
>> The current code raises a ValidationError including the list of formats.
>
> On yesterday's meeting, Simo said that on the wire and for CLI output,
> we should use generalized time.
> I disagree with using it for CLI ouptut, as I find generalized time
> unreadable, but for the API it's the obvious choice.
>
> I believe we should require the "Z" postfix in all formats, so that 1)
> users are forced to be aware that it's UTC, and 2) we can
> theoretically support local time in the future.
>
> I think the supported input formats should be:
>
> %Y%m%d%H%M%SZ      (generalized time)
> %Y-%m-%dT%H:%M:%SZ (ISO 8601, with seconds)
> %Y-%m-%dT%H:%MZ    (ISO 8601, without seconds)
> %Y-%m-%dZ          (ISO 8601, date only)
>
> I also think we should accept a space instead of the "T", as Petr²
> said above.
>

Thanks for review, everybody.

* All accepted formats now require explicit Z
* Accept values with ' ' instead of T
* LDAP generalized time used on the wire with JSON
* Minor implementation fixes

Updated patch should address all the issues.

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140225/e50ec7c3/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0137-2-ipalib-Add-DateTime-parameter.patch
Type: text/x-patch
Size: 7176 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140225/e50ec7c3/attachment.bin>