Add hook to prevent 'try_task_config.json' from being checked in on non-try trees

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

Details

Attachments

(2 attachments)

In bug 1380306, we are adding the ability to schedule which tasks to run on try via a json file checked into the tree. This way we can stop using try syntax while making it more apparent how the scheduling is working behind the scenes (scheduling info will show up in the diff).

This file is meant to be generated by tools (which should use an in-memory commit to add it), but sooner or later a developer will make this file manually and forget to delete it afterwards. So we'll need to handle this case via hook.
Which repos does this apply to? Firefox repos? Any repo with a .taskcluster.yml at the root directory?

Presumably we'll also want to exclude non-publishing and user repos.
Flags: needinfo?(ahalberstadt)
This applies to anything that uses the in-tree taskgraph generation, so presumably that's only Firefox repos.

I don't think we'll want to exclude other non-publishing repos or user repos, as committing this file to anywhere other than try will be a no-op. I guess in those cases it isn't a big deal if the file lands, but it could still cause annoyances down the line, e.g if the review repository allows it, but autoland doesn't.
Flags: needinfo?(ahalberstadt)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
I've ran ./runtests.py, but not sure if there's any other testing I should do.

Gps, can you enable this hook on the appropriate trees? I don't have a strong preference on the user repo issue.. release + integration firefox trees is probably good enough.
Comment on attachment 8887464 [details]
hghooks: add hook to prevent try_task_config.json from landing on non-try trees (bug 1380357),

https://reviewboard.mozilla.org/r/158316/#review167874

This is mostly reasonable.

At some point we should probably consolidate "run this on publishing Firefox repos" into a single hook so we don't have to manage N hooks. But that is my problem and not yours :)

::: hghooks/mozhghooks/prevent_try_config.py:17
(Diff revision 1)
> +    banner = [
> +        ' {} '.format(level.upper()).center(width, '*'),
> +        message.strip(),
> +        '*' * width,
> +    ]
> +    print('\n' + '\n'.join(banner) + '\n')

This should use `ui.write()`.

::: hghooks/mozhghooks/prevent_try_config.py:26
(Diff revision 1)
> +    if source in ('pull', 'strip'):
> +        return 0
> +
> +    for rev in repo.changelog.revs(repo[node].rev()):
> +        ctx = repo[rev]
> +        if any(f == 'try_task_config.json' for f in ctx.files()):

`'try_task_config.json' in ctx.files()` will suffice: `ctx.files()` is a list and likely always will be.
Attachment #8887464 - Flags: review?(gps) → review-
Comment on attachment 8887465 [details]
hghooks: update try syntax hook to account for the task config method (bug 1380357),

https://reviewboard.mozilla.org/r/158318/#review167876

Aside from me wanting you to tweak the file presence check, this looks fine.

::: hghooks/mozhghooks/try_warning.py:36
(Diff revision 1)
>      if source in ('pull', 'strip'):
>          return 0
>  
> -    # Block the push unless they use the try_syntax
> -    # 'try: ' is enough to activate try_parser and get the default set
> -    comment = repo.changectx('tip').description()
> +    tip = repo.changectx('tip')
> +    comment = tip.description()
> +    config_found = 'try_task_config.json' in tip.files()

Let's just look for this file in the manifest: not in the changed files list.
Attachment #8887465 - Flags: review?(gps) → review-
Comment on attachment 8887464 [details]
hghooks: add hook to prevent try_task_config.json from landing on non-try trees (bug 1380357),

https://reviewboard.mozilla.org/r/158316/#review167900
Attachment #8887464 - Flags: review?(gps) → review+
Comment on attachment 8887465 [details]
hghooks: update try syntax hook to account for the task config method (bug 1380357),

https://reviewboard.mozilla.org/r/158318/#review167902

Please flag me for needinfo to track deploying this.

I /may/ be able to do it today depending on if there are other pending changes in v-c-t (we avoid big deployments on Fridays).

::: hghooks/mozhghooks/try_warning.py:27
(Diff revision 2)
>      banner = [
>          ' {} '.format(level.upper()).center(width, '*'),
>          message.strip(),
>          '*' * width,
>      ]
> -    print('\n' + '\n'.join(banner) + '\n')
> +    ui.write('\n' + '\n'.join(banner) + '\n\n')

Thanks for fixing this too.
Attachment #8887465 - Flags: review?(gps) → review+
I assume it's safe to autoland these in the meantime, but I'll leave that up to you as well just in case.
Flags: needinfo?(gps)
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1cfdb9ef7aef
hghooks: add hook to prevent try_task_config.json from landing on non-try trees , r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a9d84ec105d9
hghooks: update try syntax hook to account for the task config method , r=gps
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
This is deployed.

Hook activated on central, inbound, autoland, beta, release, esr52. I should probably activate it elsewhere like random project branches. But these are the important ones.
Flags: needinfo?(gps)
Thanks, much appreciated! I think that's good enough, as long as people stick to using |mach try| there should be very low risk of that file being left hanging around.
You need to log in before you can comment on or make changes to this bug.