1312500 - Refactor nightly signing task a bit to better support multiple consumers

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Justin Wood (:Callek)]

This splits out stuff specific to build signing into its own transform, adds a set of validation routines for the signing transforms itself and gets rid of the duplication that existed in the nightly-l10n signing transforms.

This also moves the various dependency-grabbed injection points into just throwing the dep into the transform to be utilized as each transform needs.

Comment 1

[Justin Wood (:Callek)]

Attachment: Diff of full task graph before vs after the upcoming patch.

Demonstrates that the only changes are actually worthwhile.

Comment 2

[Justin Wood (:Callek)]

Attachment: Bug 1312500 - Improve Signing Task reusability/comprehension.

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

Comment 3

[Kim Moir [:kmoir] ET]

I appreciate the refactoring.  In many ways, it makes things clearer.  

However, I'm confused as to why we need two signing transforms, one for l10n and one for builds.  Should we just have one signing transform?  Also, this patch does not apply cleanly on the current state of date.

Comment 4

[Justin Wood (:Callek)]

(In reply to Kim Moir [:kmoir] from comment #3)
> I appreciate the refactoring.  In many ways, it makes things clearer.  
> 
> However, I'm confused as to why we need two signing transforms, one for l10n
> and one for builds.  Should we just have one signing transform?  Also, this
> patch does not apply cleanly on the current state of date.

Mostly I felt that a single transform to do the magic for 'build' signing that is common to build signing but is a seperate thing from l10n signing would be the clearest rather than try to bake the divergent logic into here which could cause confusion.

Additionally the kinds themselves, in this way, can share a common implementation (Signing) yet diverge based on transforms as needed. -- The l10n transform .py in particular is a bit confusing/annoying at present due to the chunking and prettyname needs. Rather keep that logic isolated out of the build logic to avoid extra confusion when trying to parse.

This should have applied cleanly on `date` though, it was indeed based on the date branch, I'll re-push to mozreview in a moment...

Comment 5

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Kim Moir [:kmoir] from comment #3)
> > I appreciate the refactoring.  In many ways, it makes things clearer.  
> > 
> > However, I'm confused as to why we need two signing transforms, one for l10n
> > and one for builds.  Should we just have one signing transform?  Also, this
> > patch does not apply cleanly on the current state of date.
> 
> Mostly I felt that a single transform to do the magic for 'build' signing
> that is common to build signing but is a seperate thing from l10n signing
> would be the clearest rather than try to bake the divergent logic into here
> which could cause confusion.
> 
> Additionally the kinds themselves, in this way, can share a common
> implementation (Signing) yet diverge based on transforms as needed. -- The
> l10n transform .py in particular is a bit confusing/annoying at present due
> to the chunking and prettyname needs. Rather keep that logic isolated out of
> the build logic to avoid extra confusion when trying to parse.
> 
> This should have applied cleanly on `date` though, it was indeed based on
> the date branch, I'll re-push to mozreview in a moment...

Also, yes this is based on date's:

cset: 79a2f66ff5c0

Comment 6

[Kim Moir [:kmoir] ET]

So it looks good to me, I tested it with a try run to make sure the android and linux32/64 nightlies appeared in the task graph with the expected data

Given Dustin's comments here, 

https://bugzilla.mozilla.org/show_bug.cgi?id=1171738#c7

I'd be interested in his comments on this refactor

before I r+

Comment 7

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

Comment on attachment 8803973 [details]
Bug 1312500 - Improve Signing Task reusability/comprehension.

https://reviewboard.mozilla.org/r/88162/#review87432

This looks awesome.  Please also add a paragraph or so to `taskcluster/docs/transforms.rst` about this great new "signing description" you've created.

The `r-` is for the use of underscores, but it will be a quick r+ based on the interdiff :)

::: taskcluster/ci/nightly-l10n-signing/kind.yml:9
(Diff revision 1)
>  
>  implementation: taskgraph.task.signing:SigningTask
>  
>  transforms:
>     - taskgraph.transforms.nightly_l10n_signing:transforms
> +   - taskgraph.transforms.signing:transforms

I like this model!

::: taskcluster/taskgraph/task/signing.py:31
(Diff revision 1)
>              if task.kind not in config.get('kind-dependencies'):
>                  continue
>              if not task.attributes.get('nightly'):
>                  continue
>              signing_task = {}
> -            signing_task['build-label'] = task.label
> +            signing_task['dependent_task'] = task

signing_task = {'dependent-task': task}
    
also, the keys in these dictionaries are `-` separated, not `_`.  I know it's a detail, but it's one more thing to remember about the name if we're not consistent.

::: taskcluster/taskgraph/transforms/nightly_l10n_signing.py:85
(Diff revision 1)
>  
>  @transforms.add
> -def add_signing_artifacts(config, tasks):
> -    for task in tasks:
> -        task['unsigned-artifacts'] = []
> -        product = 'android' if 'android' in task['build-platform'] else 'desktop'
> +def add_signing_artifacts(config, jobs):
> +    for job in jobs:
> +        dep_job = job['dependent_task']
> +        dep_platform = dep_job.attributes.get('build_platform')

and, yes, attributes have `_`.  I should probably change that, but do so all at once :)

::: taskcluster/taskgraph/transforms/nightly_l10n_signing.py:87
(Diff revision 1)
> -def add_signing_artifacts(config, tasks):
> -    for task in tasks:
> -        task['unsigned-artifacts'] = []
> -        product = 'android' if 'android' in task['build-platform'] else 'desktop'
> -        for locale in get_locale_list(product, task['l10n_chunk']):
> -            filename = make_pretty_name(product, task['build-platform'], locale)
> +def add_signing_artifacts(config, jobs):
> +    for job in jobs:
> +        dep_job = job['dependent_task']
> +        dep_platform = dep_job.attributes.get('build_platform')
> +
> +        job['unsigned_artifacts'] = []

dashes here, too, plz

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 8803973 [details]
Bug 1312500 - Improve Signing Task reusability/comprehension.

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

Comment 9

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

Comment on attachment 8803973 [details]
Bug 1312500 - Improve Signing Task reusability/comprehension.

https://reviewboard.mozilla.org/r/88162/#review87498

Comment 10

[Kim Moir [:kmoir] ET]

Comment on attachment 8803973 [details]
Bug 1312500 - Improve Signing Task reusability/comprehension.

removing my review flag since Dustin reviewed

Comment 11

[Justin Wood (:Callek)]

https://hg.mozilla.org/projects/date/rev/6ae07fa4b01149f1e4e61fa2d553782b84cee654