1340440 - Add a hook to reject changes to sync-messages.json without review from a IPC peer

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Once bug 1336919 lands we will want to reject such changes from the hg hook. We could extend the webidl hook or just copy and modify that to our needs.

Comment 1

[(no longer active)]

I wrote a patch for this.  Unfortunately <http://mozilla-version-control-tools.readthedocs.io/en/latest/devguide/environment.html#devguide-create-env> fails for me so I can't really run the tests.  I had to leave the changeset IDs in the test as xxx and hope that it passes.  :-)

Comment 2

[(no longer active)]

Attachment: Extend the WebIDL hook to also check for review requirements on sync-messages.ini

glob, do you mind please running the tests to see what happens, and give me the changeset sha1 that the test would expect?  Thanks!

Comment 3

[:glob ✱]

Comment on attachment 8838802 [details] [diff] [review]
Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Review of attachment 8838802 [details] [diff] [review]:
-----------------------------------------------------------------

for now i'd prefer if this hook wasn't renamed as renaming a hook requires editing all the relevant hgrc files at the same time the code lands.
can you keep it as prevent_webidl_changes.py, and file a follow-up bug to rename it to prevent_unreviewed_changes.py.
i'll chat with gps about the best way to do that without causing disruption upon his return.

i'll attach a diff of the test results.

::: hghooks/mozhghooks/prevent_unreviewed_changes.py
@@ +146,5 @@
> +            if file.endswith('.webidl'):
> +                webidlReviewed = search(DOM_authors, DOM_peers)
> +                if not webidlReviewed:
> +                        error += "WebIDL file %s altered in changeset %s without DOM peer review\n" % (file, short(c.node()))
> +                        note = "\nChanges to WebIDL files in this repo require review from a DOM peer in the form of r=...\nThis is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..\n"

the indentation is wrong here, and in the next block

Comment 4

[:glob ✱]

Attachment: test-prevent-unreviewed.diff

Comment 5

[(no longer active)]

Attachment: Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Comment 6

[(no longer active)]

Attachment: Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Comment 7

[:glob ✱]

Comment on attachment 8839735 [details] [diff] [review]
Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Review of attachment 8839735 [details] [diff] [review]:
-----------------------------------------------------------------

::: hghooks/tests/test-prevent-webidl.t
@@ +1,5 @@
>    $ hg init server
>    $ cd server
>    $ cat >> .hg/hgrc << EOF
>    > [hooks]
> +  > pretxnchangegroup.prevent_unreviewed = python:mozhghooks.prevent_unreviewed_changes.hook

oops - looks like you forgot to revert the hook name changes in the test.

Comment 8

[(no longer active)]

Attachment: Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Comment 9

[:glob ✱]

Comment on attachment 8839932 [details] [diff] [review]
Extend the WebIDL hook to also check for review requirements on sync-messages.ini

Review of attachment 8839932 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 10

[(no longer active)]

Thanks for the review! How should I get this deployed these days?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/77618d43e04e
Extend the WebIDL hook to also check for review requirements on sync-messages.ini; r=glob

Comment 12

[:glob ✱]

i should be able to deploy this early next week.

Comment 13

[(no longer active)]

Thanks!

Comment 14

[:glob ✱]

this was deployed last night.