Open Bug 1137042 Opened 5 years ago Updated 2 years ago

Define code reviewers in moz.build files

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

This work item is split off from bug 1132771. The proposal is to store code reviewers in moz.build files inside the Files context.

Will throw up the code in a minute.
Attached file MozReview Request: bz://1137042/gps (obsolete) —
/r/4343 - Bug 1137042 - Support for defining code reviewer metadata
/r/4345 - Bug 1137042 - Define some reviewers

Pull down these commits:

hg pull review -r 0c661ea96837e401626d1ac901e880088dfc56b9
https://reviewboard.mozilla.org/r/4341/#review3933

::: moz.build
(Diff revision 1)
> +        'gps@mozilla.com',
> +        'mh+mozilla@glandium.org',
> +        'mshal@mozilla.com',
> +        'ted@mielczarek.org',

E-mail addresses are probably a good ID as they correspond to Bugzilla and MozReview accounts, don't change very often, and can't be duplicated.

On the other hand, having e-mail addresses in plaintext in source files my raise some basic spam address harvesting concerns. Bugzilla mail addresses aren't publicly visible, although we do have e-mail addresses in plaintext in the Mercurial web interface for people who made at least one commit already. But Mercurial metadata is copied around less than source code.

Maybe we can apply some basic obfuscation to mitigate this concern? Maybe just write addresses in the form "gps/mozilla.com" instead.

::: python/mozbuild/mozbuild/test/frontend/data/files-metadata/reviewers/moz.build
(Diff revision 1)
> +with Files('**'):
> +    REVIEWERS += ['base1@example.com', 'base2@example.com']

I believe this will become Files('\*') per your recent recommendation.
(In reply to :Paolo Amadini from comment #2)
> I believe this will become Files('\*') per your recent recommendation.

And... can we disable the markdown checkbox by default in MozReview? :-)

Read: Files('*')
(In reply to :Paolo Amadini from comment #2)
> On the other hand, having e-mail addresses in plaintext in source files my
> raise some basic spam address harvesting concerns.

You mean like all of the plaintext emails in the top-level AUTHORS file?
(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> (In reply to :Paolo Amadini from comment #2)
> > On the other hand, having e-mail addresses in plaintext in source files my
> > raise some basic spam address harvesting concerns.
> 
> You mean like all of the plaintext emails in the top-level AUTHORS file?

Yeah, plaintext e-mails like those that were added in the past to that opt-in list.

Do we have any anecdotes about how much spam targets those addresses, a lot or not at all?
Or how about plaintext emails on the modules page? https://wiki.mozilla.org/Modules

Or how about the fact that reviewers are typically involved in public mailing lists, which can easily be harvested for email addresses.

I think it's safe to say that reviewers almost certainly have their email in the public domain already. I don't see a net reduction in privacy/spam protections here. But that shouldn't be a valid excuse.

I still stand by the strong identifying properties of email addresses as the primary reason to use emails and not some other identifier. If we had a better and trusted API for authenticating identity for things like IRC nicks, I'd use it. But AFAIK we don't. Email is the best we have. We have no better option, unfortunately.
Attachment #8569590 - Attachment is obsolete: true
bmo addresses are already considered public.  have a read of the warning on https://bugzilla.mozilla.org/createaccount.cgi
keep in mind even though you cannot see email addresses in the UI unless you're logged in, anyone can create an account so effectively there's no barrier.

> 

i like this direction - i can see a ton of value in using moz.build to store who is *allowed* to review changes, instead of using the tree-wide level3 group.


a major concern i have with this is there doesn't appear to be clear ownership of the reviewer list.  approving changes to the list can be easily handled by requiring an r+ from an existing reviewer, but there should be some mechanism for ensure that people are removed from the list within an appropriate timeframe.

for example, if someone is "asked to leave" the community, how would we prevent them from abusing r=me to autoland malicious code?  a carefully crafted change to code may not raise suspicions at a glance.


a step towards managing that could be to add OWNERS along side REVIEWERS (and make each review list require an owner).  owners would be "on the hook" to ensure lists are up-to-date, and we could generate reports to owners reminding them of the reviewers for groups they own (and as a reminder of their responsibilities).  these reports could be generated periodically, as well as triggered when a reviewer's email address or employment status changes.
We already have a hard time having an up-to-date list at a central place, I can't see how having one in-tree would make things any better. In-tree data for this is IMHO about the worst place where this could be stored. What is going to use this data? If it's in-tree scripts used by developers on their machine, then the result will depend on what they have in their local clone which may be the clone of an old branch, or contain local changes to the data. If it's things like bugzilla, what is the valid version for it to use? Current m-c? But then you can't urgently remove someone without forcing a merge of an integration branch. Top of an integration branch? But which one? What version should people consult? The one in their local tree? It's not going to be fresh.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.