1231768 - Please add a a=css-image-only verification hook

Status: RESOLVED FIXED

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

Description

[Sylvestre Ledru [:Sylvestre]]

As discussed in a thread [1], we are going to experiment a "blanket approval" for css and/or image only changes. This will start when 46 reaches mozilla-aurora (in 6 weeks)

We would need a hg hook which would check that there is only css and/or images in a commit when "a=css-image-only" as approval is used.
This hook should accept the commit only in mozilla-aurora (not in mozilla-{beta,release,esr}.

Can someone implement that or should I propose a patch?

[1] https://groups.google.com/forum/#!topic/firefox-dev/twk52oXZm6A

Comment 1

[Gregory Szorc [:gps]]

Being lazy, I would prefer someone else write this :)

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/prevent_string_changes.py is a similar hook. We require tests for new hooks. See https://mozilla-version-control-tools.readthedocs.org/en/latest/devguide/index.html for how to create the dev environment and run tests. tl;dr

  $ ./create-test-environment
  $ ./run-tests -j8 hghooks

Comment 2

[:Gijs (he/him)]

I can't get the virtualenv to work / get setup properly. The setup exited early with an error:

Building Docker images.
This could take a while and may consume a lot of internet bandwidth.
If you don't want Docker images, it is safe to hit CTRL+c to abort this.
Error running mach:

    ['build-all']

The error occurred in mach itself. This is likely a bug in mach itself or a
fundamental problem with a loaded module.

Please consider filing a bug against mach by going to the URL:

    https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=mach


If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

DockerException: Error while fetching server API version: ('Connection aborted.', error(2, 'No such file or directory'))

  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/mach/main.py", line 344, in run
    return self._run(argv)
  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/mach/main.py", line 432, in _run
    instance = cls(context)
  File "/Users/gkruitbosch/dev/version-control-tools/testing/vcttesting/docker_mach_commands.py", line 39, in __init__
    d = Docker(state_file, docker_url, tls=tls)
  File "/Users/gkruitbosch/dev/version-control-tools/testing/vcttesting/docker.py", line 152, in __init__
    self.client = docker.Client(base_url=url, tls=tls, version='auto')
  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/docker/clientbase.py", line 53, in __init__
    self._version = self._retrieve_server_version()
  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/docker/clientbase.py", line 73, in _retrieve_server_version
    'Error while fetching server API version: {0}'.format(e)
You will not be able to run tests that require Docker.
Please see https://docs.docker.com/installation/ for how to install Docker.



Opening the virtualenv works, but running ./run-tests produces the similar-looking error:

gkruitbosch-16516:version-control-tools gkruitbosch$ source venv/bin/activate
(venv)gkruitbosch-16516:version-control-tools gkruitbosch$ ./run-tests -j8 hghooks
Traceback (most recent call last):
  File "./run-tests", line 163, in <module>
    docker = vctdocker.Docker(docker_state, docker_url, tls=docker_tls)
  File "/Users/gkruitbosch/dev/version-control-tools/testing/vcttesting/docker.py", line 152, in __init__
    self.client = docker.Client(base_url=url, tls=tls, version='auto')
  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/docker/clientbase.py", line 53, in __init__
    self._version = self._retrieve_server_version()
  File "/Users/gkruitbosch/dev/version-control-tools/venv/lib/python2.7/site-packages/docker/clientbase.py", line 73, in _retrieve_server_version
    'Error while fetching server API version: {0}'.format(e)
docker.errors.DockerException: Error while fetching server API version: ('Connection aborted.', error(2, 'No such file or directory'))


I'll attach my WIP patch for now, and if people have feedback about how to make this work I can try again.

Comment 3

[:Gijs (he/him)]

Attachment: WIP untested patch

Comment 4

[:Gijs (he/him)]

Hrmpf, did not intend to take this if I can't guarantee I can push this over the line.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8698484 [details] [diff] [review]
WIP untested patch

I believe the Docker issue was fixed. It was a recent regression in the test environment.

Please resubmit this via MozReview for me to look at it.

Comment 6

[:Gijs (he/him)]

(In reply to Gregory Szorc [:gps] from comment #5)
> Comment on attachment 8698484 [details] [diff] [review]
> WIP untested patch
> 
> I believe the Docker issue was fixed. It was a recent regression in the test
> environment.

I just updated and I'm still getting exactly the same error. I'll file a separate bug, I guess. :-\

> Please resubmit this via MozReview for me to look at it.

I would do that if I were at least able to run the thing to make sure it wasn't completely busted. I can't right now. Hence feedback? rather than review?, a distinction mozreview doesn't support.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch (away 18-28 Dec.) from comment #6)
> (In reply to Gregory Szorc [:gps] from comment #5)
> > Comment on attachment 8698484 [details] [diff] [review]
> > WIP untested patch
> > 
> > I believe the Docker issue was fixed. It was a recent regression in the test
> > environment.
> 
> I just updated and I'm still getting exactly the same error. I'll file a
> separate bug, I guess. :-\

Sorry, it does seem the error is slightly different - but it still doesn't work. Filed bug 1232982.

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1231768 - add hook for css-image-only-approvals, r?gps

Review commit: https://reviewboard.mozilla.org/r/28167/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28167/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8698985 [details]
MozReview Request: Bug 1231768 - add hook for css-image-only-approvals, r?gps

https://reviewboard.mozilla.org/r/28167/#review25211

It isn't clear to me how this hook is intended to interact with the existing approval hook. You probably want to enable both hooks in tests for each to ensure interactions are appropriate. Or, consider inlining this hook's functionality into the existing approval hook.

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:27
(Diff revision 1)
> +    changesets = list(repo.changelog.revs(repo[node].rev()))
> +    if not ('a=css-image-only' in repo.changectx(changesets[-1]).description().lower()):

I don't see the value in the `changesets` variable. You can access the tip-most changeset via `repo['tip']`

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:28
(Diff revision 1)
> +    if not ('a=css-image-only' in repo.changectx(changesets[-1]).description().lower()):

Python has a "not in" operator which is preferred for these things.

`if 'foo' not in bar:`

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:32
(Diff revision 1)
> +    error = ""

Nit: I'm not a huge fan of string concatenation because it isn't as optimized in Python as it is in JavaScript.

Please use the following pattern:

   errors = []
   errors.append('...')
   print('\n'.join(errors))

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:36
(Diff revision 1)
> +        for file in repo[change_id].files():

"file" shadows a built-in global in Python. Please use "f" or "filename" or something else instead.

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:42
(Diff revision 1)
> +        print "\n************************** ERROR ****************************\n"

I think the trailing newline can be dropped.

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:42
(Diff revision 1)
> +        print "\n************************** ERROR ****************************\n"
> +        print error
> +        print "You used the a=css-image-only approval message, but your change"
> +        print "included non-CSS/image/jar.mn changes. Please get 'normal'"
> +        print "approval from release management for your changes."
> +        print "*************************************************************\n"

Please use print() (a function). It is compatible with future versions of Python.

Comment 10

[:Gijs (he/him)]

Note that a lot of these things were copy-pasted from the string-changes hook, so it's surprising to me that so much boils down to "use different python". Maybe the rest of the hooks need a once-over or something? :-\

Comment 11

[Gregory Szorc [:gps]]

Many of the hooks were written several years ago and it shows. They do need a once-over. It's not the highest on the priority list.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8698985 [details]
MozReview Request: Bug 1231768 - add hook for css-image-only-approvals, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28167/diff/1-2/

Comment 13

[:Gijs (he/him)]

(I believe I fixed everything you noted, but please let me know if there's anything else)

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8698985 [details]
MozReview Request: Bug 1231768 - add hook for css-image-only-approvals, r?gps

https://reviewboard.mozilla.org/r/28167/#review25329

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:2
(Diff revision 2)
> +# Copyright (C) 2014 Mozilla Foundation

2015

::: hghooks/mozhghooks/verify-css-image-only-approvals.py:40
(Diff revision 2)
> +    if len(errors) > 0:

if errors

Comment 15

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/hgcustom/version-control-tools/rev/834cf197846c

I assume that we'd still need to file an infra bug to get it enabled on aurora for the 46 cycle when we hit that.

Comment 16

[Gregory Szorc [:gps]]

We no longer need separate infra bugs for deployments. But since this one is closed, either reopen it or file a new one to track installing the hook.