Code review comment for lp:~zyga/checkbox/fix-1401996

Revision history for this message
Daniel Manrique (roadmr) wrote :

Command to test:
plainbox -C -T plainbox.ctrl run -i 2013.com.canonical.certification::cdimage

Before the patch:
INFO plainbox.ctrl: Storing resource record '2013.com.canonical.certification::cdimage': Resource({'official': 'Alpha\narchitecture: amd64\ncodename: Trusty Tahr\nrelease: 14.04 LTS\ndate: 20140304\ndistributor: Ubuntu'})

After:
INFO plainbox.ctrl: Storing resource record '2013.com.canonical.certification::cdimage': Resource({'date': '20140304', 'codename': 'Trusty Tahr', 'architecture': 'amd64', 'distributor': 'Ubuntu', 'official': 'Alpha', 'release': '14.04 LTS'})

so +1 on functionality, and the code is quite clear and easy to follow (yay for a nice, loopy algorithm). My only request would be some sort of automated testing to ensure this doesn't bite us again, but I leave that up to you for reasons discussed on IRC (because... reasons).

review: Approve

« Back to merge proposal