Code review comment for lp:~mvo/pkgme-devportal/i386-only-for-now

On Thu, Jul 19, 2012 at 05:15:33PM -0000, James Westby wrote:
> Review: Needs Information
>
> Hi Michael,
Hi James,

> I'm not really sure what "supported" architectures mean here, could you help me
> out?

Just to clarify, do you mean that the string for the configuration
option in the configglue part is not clear? Or that its overall not
clear?

What I mean with "supported" for the architectures is that those are
the architectures we have symbols for in the database and expect to
generate packages for. Do you think a different name would make it
clearer? Maybe "known" or something like this?

> 37 + # XXX: mvo: it seems we don't need "all" here as we are interessted
> 38 + # in binary symbols only?
>
> I'm not sure what that means, but it sounds like something we should have an
> answer for before we land this?

Indeed, its based on the previous code that remove "all" from the list
of ARCHITECTURES for the launchpad querry. And it made me wonder if we
need "all" at all. I think not, but maybe I'm overlooking something.

> 53 +ARCHITECTURES = load_configuration().options.architectures_supported
>
> Setting a module-level variable based on reading a config file is bad form,
> because it will read the config file at import, which means it's hard
> to write tests for example. Querying the config at use would be better.

This is fixed now in my branch.

Thinking a bit more about the branch it may actually not be that
useful, I mainly did it in preparation for the
lp:~mvo/pkgme-devportal/raise-on-unsupported-architectures

But given that the database currently does not have a architecture
field afaik it may well be a bit pointless to make it configurable
before it is actually configurable. So I'm fine putting that branch on
hold for now until the rest of the code is updated to work with
multiple architectures too.

Cheers,
 Michael