[Linaro-validation] code review: lava android benchmark code

[Zygmunt Krynicki]

On Tue, Feb 7, 2012 at 6:47 AM, Paul Larson <paul.larson at linaro.org> wrote:
> Zygmunt, have you had a chance to review the changes that Andy pointed at
> below?  It's a bit different from the merge proposal process we are
> accustomed to, but pretty easy to sort out.  Andy can correct me if I'm
> wrong, but I think you can just sign in with your linaro google id, setup
> the account, and review the changes.

I tried to review it over the weekend on my phone but the UI was
spawning new windows all the time in a loop so I deferred. I'm doing
the review now. I've never used gerrit in the past. I hope I get it
right.

> If there are no blockers, any chance we can roll this into production
> tomorrow?  Andy has his demo in the afternoon, so it would be nice if we can
> get him setup by then.  He may require staff access at least so that he can
> access the admintool.

I'll sync with Andy and try to get this deployed.

>
> Thanks,
> Paul Larson
>
> On Fri, Feb 3, 2012 at 3:29 PM, Andy Doan <andy.doan at linaro.org> wrote:
>>
>> Thanks for the review. You've taught me about some interesting things in
>> Django. Comments in-line.
>>
>> On 02/03/2012 03:24 PM, Zygmunt Krynicki wrote:
>> > Hi Andy.
>> >
>> > I'm reading the code now. I have some comments. Essentially before this
>> > lands you _have_ to add one change, then we can follow up with iterative
>> > improvements.
>> >
>> > You _have_ to apply south migrations to your application. Without that
>> > we will not be able to make reasonable upgrades. Adding migrations later
>> > is not an option as there is a bit that is very hard to automate.
>> >
>> > Fortunately this is pretty easy:
>> >
>> > 1) Add a dependency on south (install_requires in setup.py)
>> > 2) Run lava-server manage ... android_benchmark_views_app
>> > schemamigration --initial --auto
>> > 3) Inspect the code briefly and add it to version control
>> > 4) Push that up for review.
>>
>> I have a patch ready for review:
>>  http://review.android.git.linaro.org/#change,1463
>>
>>
>> > As for the rest:
>> >
>> > *) helpers.py, don't use double leading underscore.
>>
>> http://review.android.git.linaro.org/#change,1464
>>
>> > *) Try keeping the code that relates to your model in the model class,
>> > also add a few one liners if you can to document each public method
>>
>> I added some comments to models.py to help explain things:
>>  http://review.android.git.linaro.org/#change,1468
>>
>> > *) extension.py, in def version(), pass the second argument to
>> > versiontools.format_version() please pass your main package object
>> > (android_benchmark_views_app). This will make it follow git version
>> > hashes.
>>
>> I'm not positive I did what you were wanting, but:
>>
>>  http://review.android.git.linaro.org/#change,1465
>>
>> > *) models.py, BenchmarkRun, each ForeignKey should have a related_name.
>> > This name is used when accessing the model field from the "remote" end,
>> > then you can navigate back using this name. In general this is required
>> > in some cases (when there are clashes, anyway this is beyond the scope
>> > of this conversation)
>>
>> http://review.android.git.linaro.org/#change,1466
>>
>> > *) models.py BenchmarkReport, don't use comments like that. Firs of all,
>> > in django it's very very bad to use nullable text/char fields. Instead
>> > always use null=False, blank=True - that makes writing code then saner.
>> > As for comments. Instead of using a simple text field please consider
>> > using django comment system. It _is_ slightly more complicated than
>> > using a free-for-all text area (like on launchpad's whiteboard but at
>> > the same time you get much more in return. You can read about this here:
>> > https://docs.djangoproject.com/en/dev/ref/contrib/comments/
>>
>> So for now I've just changed the nullable to be false. I'm not sure the
>> django comment system is what we need for this. Basically in this case I
>> intend "comments" to be where you can put a bit of an "executive
>> summary" for the given report. The change I made added some help_text to
>> the field to help convey that.
>>
>>  http://review.android.git.linaro.org/#change,1470
>>
>> Let me know if you think this is adequate or if the comment system still
>> would be better. It looks easy enough to hook up.
>>
>> > *) models.py both classes, add a get_absolute_url() method based on
>> > django permalink system. It's trivial to use, see this document:
>> >
>> > https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.permalink
>>
>> http://review.android.git.linaro.org/#change,1467
>>
>> > *) You ship query.flot. While that's okay for now I'd rather see this
>> > managed by a dedicated django app that just wraps this in one place. As
>> > for now this can stay as-is.
>>
>> Yeah, that probably makes sense. My versions have some handy fixes that
>> took me a while to come up with, but would be useful to others wanting
>> to do some charting.
>>
>> We (read I) should probably look into submitting patches for this, but I
>> see so many flot patches floating around that I assume the maintainers
>> probably aren't interested.
>>
>> > *) Your templates extend dashboard templates. Please don't do that (as
>> > those templates are not a stable/public interface). Instead extend
>> > lava-server templates that work just as well but are stable.
>>
>> http://review.android.git.linaro.org/#change,1469
>>
>> >
>> > That's a very quick review. I have one more comment.
>> >
>> > EXCELLENT WORK :-)
>>
>> I cheated and tried to copy all the great work your team has done.
>>
>> _______________________________________________
>> linaro-validation mailing list
>> linaro-validation at lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-validation
>
>