feat: add scriptworker signing transforms #7
Conversation
ahal
force-pushed
the
signing
branch
5 times, most recently
from
ca89128 to
2ed6b64
Compare
|
As this is the first code being added, I think it's reasonable to have coverage decrease a bit from 100% :p |
| "build-type", | ||
| "level", | ||
| { | ||
| Required("format"): optionally_keyed_by( |
There was a problem hiding this comment.
It looks like we sometimes sign with multiple formats at once. Do you want to handle this now, or is that going to come later?
There was a problem hiding this comment.
I think that will come later. For now it's easier for me if I just focus on a single project at a time (firefox-android). But thanks for the heads up that this will be needed at some point.
| @transforms.add | ||
| def filter_out_ignored_artifacts(_, tasks): | ||
| for task in tasks: | ||
| ignore = task["signing"].get("ignore-artifacts") |
There was a problem hiding this comment.
Is there a reason we have to ignore specific things (as opposed to having the upstream tasks explicitly define what needs signing)? It's easier for debugging and other inspection to have the things we're actually going to be signing as the explicit part IMO. Not a blocker (and I'm guessing there's some reason it has to be done this way...), just curious.
There was a problem hiding this comment.
Hm, yes this is a good point.. I did it this way simply because the transforms in firefox-android use a list of extensions to ignore and adjusting the logic from that to a regex to ignore was trivial.. However, you are absolutely correct that being explicit about what we are signing seems much better than implicitly signing everything! So I will make this a blocker!
Do you think we should always enforce being explicit about which artifacts are signed? Or should it be an optional key and default to signing all artifacts? I'm leaning towards the former.
Maybe we could allow either a list of upstream artifacts to sign, or a regex whereupon any upstream artifact that matches will be signed.
There was a problem hiding this comment.
I think the most important part is that it should be an allowlist instead of a denylist. With that in mind, I think an explicit list of names, a regex of things to sign, or even a "sign all artifacts" mode are all fine.
So - what you're proposing sounds great!
There was a problem hiding this comment.
Thanks for the feedback, that's helpful. I think instead of rushing this PR into the module, I'm going to try and go straight to doing things properly by fixing parts of taskcluster/taskgraph#227 first.
Because if we're going to need the artifact names up front, then we'll require multi_dep-like logic to resolve them. Initially I was going to move multi_dep (or similar) over later, but now I'm seeing that we should do it first.. So this PR is going to be on hold for a little bit while I sort that out over in the Taskgraph repo..
I've been testing this locally in conjunction with:
mozilla-mobile/firefox-android#1648
Using this PR from that PR, results in identical taskgraphs across all checked-in parameters in
firefox-android. This PR should also be usable bymozilla-vpn-clientwith some minor changes, but I'm going to focus on Android for now.