1462791 - Refactor some taskgraph VCS code

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Gregory Szorc [:gps]]

Creating a bug to hold review of some patches I have sitting around.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1462791 - Add helper function for VCS checkout on generic-worker;

This functionality was implemented at least 3 times. Let's consolidate
it to a central function.

Returning multiple command strings is kind of funky. I preserved
existing behavior and mozharness jobs are the only ones printing the
forensic logging. We should probably move this logging into
robustcheckout so we don't need to involve taskgraph with this. But
that can be deferred to another day.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1462791 - Remove GECKO_HEAD_REF and COMM_HEAD_REF;

`hg robustcheckout` supports checking out a specific revision hash or
a symbolic revision name. These are defined by the environment
variables GECKO_HEAD_REV and GECKO_HEAD_REF, respectively.

In CI, we should *always* be using full revisions because they are
non-ambiguous (a symbolic name can change over time whereas hashes
can't).

AFAICT we are not actually using symbolic names and GECKO_HEAD_REF
anywhere. So this commit drops defining and consuming it. COMM_HEAD_REF
was nuked as well because it is in the same boat.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1462791 - Remove docker_worker_add_gecko_vcs_env_vars();

support_vcs_checkout() always sets the environment variables that
were set by this deleted function. In addition, support_vcs_checkout()
also adds caches and scopes - at least for docker-worker and
docker-engine. For generic-worker - which was used in all call sites
of docker_worker_add_gecko_vcs_env_vars() (yes, the "docker_worker"
bit of the name was completely wrong - probably a legacy holdover) -
support_vcs_checkout() was *almost* exactly equivalent to
docker_worker_add_gecko_vcs_env_vars(). The only difference is that
support_vcs_checkout() adds the
secrets:get:project/taskcluster/gecko/hgfingerprint scope in addition
to setting the environment variables.

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

Comment 4

[Gregory Szorc [:gps]]

aki: I flagged you for review because I wasn't sure if CoT relies on the *_HEAD_REF environment variables somehow.

Comment 5

[Aki Sasaki (not active)]

(In reply to Gregory Szorc [:gps] from comment #4)
> aki: I flagged you for review because I wasn't sure if CoT relies on the
> *_HEAD_REF environment variables somehow.

It looks like we only use *_HEAD_{REV,REPOSITORY} in CoT verification. If we get a try push, I can try verifying various tasks in the push via the `verify_cot` tool, but I'm not too worried.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8977105 [details]
Bug 1462791 - Add helper function for VCS checkout on generic-worker;

https://reviewboard.mozilla.org/r/245184/#review251526

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8977106 [details]
Bug 1462791 - Remove GECKO_HEAD_REF and COMM_HEAD_REF;

https://reviewboard.mozilla.org/r/245186/#review251528

::: commit-message-ec52a:13
(Diff revision 1)
> +non-ambiguous (a symbolic name can change over time whereas hashes
> +can't).
> +
> +AFAICT we are not actually using symbolic names and GECKO_HEAD_REF
> +anywhere. So this commit drops defining and consuming it. COMM_HEAD_REF
> +was nuked as well because it is in the same boat.

The cron hook uses a symbolic ref ('default').  I think this change would break such tasks.

For example:

https://tools.taskcluster.net/hooks/project-releng/cron-task-projects-holly

```json
    "env": {
      "GECKO_BASE_REPOSITORY": "https://hg.mozilla.org/mozilla-unified",
      "GECKO_HEAD_REPOSITORY": "https://hg.mozilla.org/projects/holly",
      "GECKO_HEAD_REF": "default",
...
},
```

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8977107 [details]
Bug 1462791 - Remove docker_worker_add_gecko_vcs_env_vars();

https://reviewboard.mozilla.org/r/245188/#review251530

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8977105 [details]
Bug 1462791 - Add helper function for VCS checkout on generic-worker;

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8977107 [details]
Bug 1462791 - Remove docker_worker_add_gecko_vcs_env_vars();

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/143be456f420
Add helper function for VCS checkout on generic-worker; r=dustin
https://hg.mozilla.org/integration/autoland/rev/3409c789902c
Remove docker_worker_add_gecko_vcs_env_vars(); r=dustin

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/143be456f420
https://hg.mozilla.org/mozilla-central/rev/3409c789902c

Comment 13

[Phabricator Automation]

Attachment: Bug 1462791: [taskgraph] Return list from `generic_worker_hg_commands`; r?dustin

Some code[1] expects to be able to `.extend` the result.

[1] https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/taskcluster/taskgraph/transforms/job/mozharness.py#318-331

Comment 14

[Phabricator Automation]

Comment on attachment 8980601 [details]
Bug 1462791: [taskgraph] Return list from `generic_worker_hg_commands`; r?dustin

Dustin J. Mitchell [:dustin] pronoun: he has approved the revision.

https://phabricator.services.mozilla.com/D1384

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/fe66c9e30a20
[taskgraph] Return list from `generic_worker_hg_commands`; r=dustin a=Aryx