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
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; consumers handle it before delegating here since it depends on gecko-specific scriptworker code.
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 |
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).