Code review comment for lp:~pfalcon/linaro-license-protection/crowd-auth

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

> I think it makes sense to add warning that user is using "old" version of BUILD-INFO.txt with OpenID-Launchpad-Teams which will be deprecated sooner or later.

Yes, makes sense, I added warning this may happen in the future to README. But when it should happen? It should happen during publishing stage, not in webapp. Or do you hint about updating scripts/publish_to_snapshots.py? Well, that's not directly related with the aim of this patch, which is "adding Crowd API support to l-l-p webapp", and would be just scope creep even further. So, let's leave this for separate patch.

> Another is about tests. May be it is worth renaming test 'test_apply_to_dir(self)' to something like 'test_apply_to_dir_openid_launchpad_teams_field(self)' to better reflect what it does after you've added backwards compatibility with renaming of fields.

Well, that test actually tests how BUILD-INFO.txt applies to dirs, nothing else. It's just that was the only test in testsuite which deals with group field, so I selected it as a template for Auth-Groups field test. It's not ideal, but the whole testsuite is not ideal (well, I'm not sure it's the aim - it just should provide test coverage). Improving that should be a separate effort again.

> There is a function that does write BUILD-INFO.txt from an array, it's used in splicing, it writes out Format-Version: 0.1

Fixed, thanks.

« Back to merge proposal