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

RESOLVED FIXED

Status

Developer Services
Mercurial: hg.mozilla.org
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: Gijs)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 3

2 years ago
Created attachment 8698484 [details] [diff] [review]
WIP untested patch
Attachment #8698484 - Flags: feedback?(gps)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Hrmpf, did not intend to take this if I can't guarantee I can push this over the line.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW

Comment 5

2 years ago
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.
Attachment #8698484 - Flags: feedback?(gps)
(Assignee)

Comment 6

2 years ago
(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.
(Assignee)

Updated

2 years ago
Depends on: 1232982
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
Created attachment 8698985 [details]
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/
Attachment #8698985 - Flags: review?(gps)

Comment 9

2 years ago
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.
Attachment #8698985 - Flags: review?(gps)
(Assignee)

Comment 10

2 years ago
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? :-\
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.
(Assignee)

Comment 12

2 years ago
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/
Attachment #8698985 - Flags: review?(gps)
(Assignee)

Comment 13

2 years ago
(I believe I fixed everything you noted, but please let me know if there's anything else)
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
Attachment #8698985 - Flags: review?(gps) → review+
(Assignee)

Comment 15

2 years ago
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
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.
(Assignee)

Updated

2 years ago
Blocks: 1242630
You need to log in before you can comment on or make changes to this bug.