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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
