Bug 2052985 - feat: add build_signing transform and artifact signing specs - #237
Bug 2052985 - feat: add build_signing transform and artifact signing specs#237kryoseu wants to merge 1 commit into
Conversation
d1c2ace to
af00c41
Compare
kryoseu
changed the title
kryoseu
changed the title
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
I'm not certain how I feel about this. This is a forklift of existing gecko_taskgraph code into this repository, and it retains a lot of Gecko-isms. I don't want to let perfect by the enemy of good, but I would also prefer that we don't copy hacks into what we expect to be the long term home for this code.
I'm also not sure that I'm fond of the idea of putting the whole build_signing transform in here. That's a transform that is highly specialized, and unlikely to be generally useful, as it is very closely tied to a couple of specific kinds in gecko. A rename or possibly refactor of it might make me a little bit more comfortable with the idea.
One idea (which would also help in getting rid of some of the Gecko-isms) would be to not import this code as a transform, but pull out the core bits and have them called in transforms in both gecko and comm repositories. For example, the core bit of add_signed_routes is the inner-most for loop. Putting that in its own (pure) function would avoid the need for code here to know anything about the shippable attribute or enable-signing-routes.
I have some specific comments below, but I also think you may want to consider putting some of this code in comm instead of here. If you want a more immediate path forward, I strongly urge you to put the code you need into the comm repo for now.
| enable_signing_routes = job.pop("enable-signing-routes", True) | ||
|
|
||
| job["routes"] = [] | ||
| if dep_job.attributes.get("shippable") and enable_signing_routes: |
There was a problem hiding this comment.
shippable is a Gecko (and I guess comm) specific concept that doesn't belong here IMO.
| job["routes"] = [] | ||
| if dep_job.attributes.get("shippable") and enable_signing_routes: | ||
| for dep_route in dep_job.task.get("routes", []): | ||
| if not dep_route.startswith("index.gecko.v2"): |
There was a problem hiding this comment.
gecko in the index route is also Gecko specific. I'm not sure this would even work for comm in its current form?
|
|
||
|
|
||
| def is_partner_kind(kind): | ||
| if kind and kind.startswith(("release-partner", "release-eme-free")): |
There was a problem hiding this comment.
These are Gecko-isms that shouldn't live here (the same goes for names of other specific kinds).
| return True | ||
|
|
||
|
|
||
| def generate_specifications_of_artifacts_to_sign( |
There was a problem hiding this comment.
This function in particular I do not think should move here in its current form. It has...not aged well to say the least, and I would prefer not to prolong its life.
jcristau
left a comment
jcristau
left a comment
There was a problem hiding this comment.
Agree with Ben that this shouldn't be moved, or not as-is.
|
I think it's worth stepping back and discussing what actually is the thing we want to share across all projects here. When it comes to signing, the mechanism we use is the In gecko (and comm), the thing that makes task payloads conform to the schema is already in So instead of copy/pasting transforms, I think the question we want to ask is what logic would make sense to tease out of the various signing transforms (across gecko, comm, and even other repos) and place into |
This adds `transforms/build_signing.py` with `add_signed_routes` and `define_upstream_artifacts`. The index route prefix is read from `graph_config` (`scriptworker.signed-route-prefix`) instead of hardcoding `index.gecko.v2`, and the set of artifacts to sign comes from a project-populated `job["signing-artifacts"]` key rather than from Firefox-specific product data. A project that configures no prefix gets no signed routes, so this is inert for existing consumers. Also teaches `build_signing_payload` to honour `scriptworker.excluded-signing-formats`. Thunderbird has no way to say up front which formats it doesn't want, so today it strips them back out after the payload is already built. Three transforms in `comm/taskcluster/comm_taskgraph/transforms/signing.py` do this: - remove_widevine - no_sign_langpacks - check_for_no_formats They edit `upstreamArtifacts` to remove the widevine and langpack formats that Firefox added, and drop any artifact left with nothing to sign. This turns the first two into a config setting, which would let comm delete them. An artifact whose formats are emptied by the exclusion is dropped; one that arrived with no formats is kept, since notarization kinds emit those. That is deliberately not what check_for_no_formats does, so it is left alone here. Bug 2052985
af00c41 to
7486a9e
Compare
4 participants
Port the desktop and source build-signing transforms (add_signed_routes, define_upstream_artifacts) and the shared generate_specifications_of_artifacts_to_sign helper from gecko_taskgraph so they can be consumed by both Gecko and comm.
Android signing is left out from
generate_specifications_of_artifacts_to_sign. Consumers handle it before delegating here since it depends on gecko-specific scriptworker code (generate_beetmover_upstream_artifacts).