1247072 - Disable the pre-commit try syntax hook

Status: RESOLVED FIXED

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

Description

[Andrew Halberstadt [:ahal]]

With the new "Add new Jobs" functionality in treeherder (and with other mozci based tools), pushing to try without a try syntax is a very valid workflow.

The only problem is that the hook prevents this, so we still need to use a dummy syntax like 'try: none' to get around it. Let's drop the hook and let people fill in their jobs later via treeherder or mozci if they want.

Note, we'll need to verify that a push to try with no try syntax is equivalent to DONTBUILD. This might already be the case.

Comment 1

[Gregory Szorc [:gps]]

We should keep the hook, but it should issue a warning that no jobs will be scheduled if no try syntax is detected.

If you want to have a go at this, the code is at https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/try_mandatory.py.

$ ./create-test-environment
$ ./run-tests -j4 hghooks/tests/test-try-mandatory.t

will run the tests.

Comment 2

[Andrew Halberstadt [:ahal]]

Keeping the hook as a warning is even better. Also try: none actually appears to be equivalent to try: -b do -p all, so the workflow from comment 0 isn't even possible. Looking at the try_parser code, we'll likely need to do some prerequisite work there before this bug.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: hghooks: allow pushing to try without a try syntax (bug 1247072), r?gps

Try syntax is no longer required to schedule jobs on try. This can now be accomplished
directly in treeherder (or via other mozci based tools). This changes the try hook to
allow pushes with no try syntax. For now, print a verbose WARNING so people understand
what's happening. We may want to tone the warning down a bit if this workflow becomes
more standard in the future.

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

Comment 4

[Andrew Halberstadt [:ahal]]

Please don't land this just yet. We'll want to wait for bug 1247085 to land first, and I want to overhaul https://wiki.mozilla.org/Try

Comment 5

[Gregory Szorc [:gps]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Please don't land this just yet. We'll want to wait for bug 1247085 to land
> first, and I want to overhaul https://wiki.mozilla.org/Try

As the scheduling logic will eventually all live in tree, I would be tempted to document Try syntax in tree ;)

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8723573 [details]
MozReview Request: hghooks: allow pushing to try without a try syntax (bug 1247072), r?gps

https://reviewboard.mozilla.org/r/36623/#review33323

Nice. FWIW, the misc taskcluster task support I added basically schedules non-platform tasks. So `-p none` does kinda make sense, since I /think/ the default is `-p all`. This is something we'll want to fix. I won't make you scope bloat it. If you do, look for `-j/--job <x>` in the try syntax and error for `-p none` if it isn't present.

Comment 7

[Andrew Halberstadt [:ahal]]

Maybe I should just remove the check for -p none? There are lots of ways to mess up your try syntax.. I found it a bit odd that this was the only one the hook was bothering to catch.

Comment 8

[Gregory Szorc [:gps]]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Maybe I should just remove the check for -p none? There are lots of ways to
> mess up your try syntax.. I found it a bit odd that this was the only one
> the hook was bothering to catch.

WFM.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8723573 [details]
MozReview Request: hghooks: allow pushing to try without a try syntax (bug 1247072), r?gps

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

Comment 10

[Andrew Halberstadt [:ahal]]

Figured might as well just add the check.. not like it's hard or anything.

Comment 11

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/12e31fae88f4

Gps, when does this go live?