launchpad-reviewers team mailing list archive

[Diogo Matsubara <matsubara@xxxxxxxxxxxxx>]

On Tue, Oct 27, 2009 at 2:48 PM, Gary Poster <gary.poster@xxxxxxxxxxxxx> wrote:
> Review: Needs Fixing
> Looks good!  A few questions.  If it is something that I would really like you to address before you land the branch, it is preceded by an asterisk.
>
> For my own curiosity, what does this do (from the Makefile)? DJANGO_SETTINGS_MODULE=oopstools.settings

It sets the environment variable DJANGO_SETTINGS_MODULE to point to
the settings used by this Django project. Without it I get something
like:

    raise ImportError("Settings cannot be imported, because
environment variable %s is undefined." % ENVIRONMENT_VARIABLE)
ImportError: Settings cannot be imported, because environment variable
DJANGO_SETTINGS_MODULE is undefined.

For some of my scripts, like load_sample_data.py, I use some of the
infrastructure provided by Django to load the data in the DB and
that's how I tell Django where the settings.py file is.

>
> I suggested that we add appinstance and vhost to the Ooops Metadata model.  I thought we would get them from the url.  It see now that we can just get appinstance from the oops itself, so that is simpler.  Is it valuable to make the vhost set also?

I don't think so. The only case I'm using the vhost information is to
figure out which team the oops belongs to. Having it on the
OopsMetaData would allow us to filter on that information as well, but
what really interest us now is the mapping between Vhost -> Team

>
> * I'm sure you are familiar with the XXX policy for Launchpad--most notably, an XXX must reference a bug.  I'd be tempted to make the same rule for our other projects, like the oops tools.  Could you add a bug for that XXX on line 554 of the diff, or just fix it?  To recap, my biggest concern is that line 570 treats the "no match" condition as "foundations," and line 580 treats the "no match" condition as "unknown."  I have related thoughts, but if you address that one, that will be good enough to remove the XXX.  (Again, if you don't want to address it now, turn it into a bug.)

Removed the XXX and made the code return the unknown team for cases
where we have a vhost-less url and when we can't figure out the team
by the oops prefix

>
> * For the XXX on line 568, do you think there's a heuristic possible to solve this?  Basically, my  question comes down to "if it is worthy of a bug, make a bug; otherwise figure out how you can feel comfortable removing the XXX."
>

Not really. It's similar to the prefix situation, we don't have enough
information to guess the right team, so I the correct thing to do is
to make them go to the 'unknown' team.

> * I strongly suspect that the approach to handling no-match teams and appinstances is not efficient.  If you don't want to filter by something, don't include it as a query term.  You could accomplish this within the skeleton of your current approach by having a kwargs dict that you begin in that block.  If you get a team match, add it to the kwargs (e.g., ``kwargs['team__in'] = [team.id]``).  Then, when you call filter, use **kwargs.

Done per our conversation on IRC.

>
> That's good for this branch.  Let me know if you'd like a sketch of any of these changes.
>
> Thanks!

Thank you for reviewing. Just pushed r136 addressing your comments above.
-- 
Diogo M. Matsubara
https://code.launchpad.net/~matsubara/oops-tools/bug-417108-vhost-separation/+merge/14034
Your team Launchpad code reviewers is subscribed to branch lp:~launchpad-pqm/oops-tools/trunk.