launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
> --- lib/lp/snappy/model/snapbuildbehaviour.py	2019-03-06 23:00:49 +0000
> +++ lib/lp/snappy/model/snapbuildbehaviour.py	2019-04-30 12:55:54 +0000
> @@ -142,6 +142,14 @@
>                  (build.snap.owner.name, build.snap.name))
>          args["build_source_tarball"] = build.snap.build_source_tarball
>          args["private"] = build.is_private
> +        build_request = build.build_request
> +        if build_request is not None:
> +            # RFC3339 format for timestamp
> +            # (matching snapd, SAS and snapcraft representation)
> +            timestamp = build_request.date_requested.replace(
> +                microsecond=0, tzinfo=None).isoformat() + 'Z'

It bothers me slightly that this code is repeated almost verbatim in the test, but test_extraBuildArgs_build_request_args is probably the wrong place to be testing the timestamp format.  Do you think you could do something like this?

 * split out a format_as_rfc3339 helper function similar to that in SCA (it can just be near the top of this file for now)
 * use format_as_rfc3339 in test_extraBuildArgs_build_request_args
 * borrow the format_as_rfc3339 tests from SCA with whatever small adjustments are needed, so that we have some tests in LP that say something about the timestamp format

> +            args["build_request_id"] = build_request.id
> +            args["build_request_timestamp"] = timestamp
>          defer.returnValue(args)
>  
>      @defer.inlineCallbacks


-- 
https://code.launchpad.net/~matiasb/launchpad/snap-build-request-extra-args/+merge/366594
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.