Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for macapp format #23

Merged
merged 2 commits into from
Dec 10, 2020
Merged

Conversation

bhearsum
Copy link
Contributor

@bhearsum bhearsum commented Dec 9, 2020

I think this is pretty straightforward. The only thing I really want to call out is that I'm pretty sure we can use the same worker for level 1 & level 3 signing. RelEng are the only people who can initiate these requests, so I don't see any security benefit to having separate workers for dep vs release. If I'm wrong or missing something, just let me know.

Unrelated to the work here, the first commits removes what appears to be an unused part of the config (we tried to put manifests elsewhere at one point it looks like? but never used them?)

@escapewindow
Copy link
Contributor

I think this is pretty straightforward. The only thing I really want to call out is that I'm pretty sure we can use the same worker for level 1 & level 3 signing. RelEng are the only people who can initiate these requests, so I don't see any security benefit to having separate workers for dep vs release. If I'm wrong or missing something, just let me know.

Right. We'll need to manually switch over the worker to level 1 if we want to test, or spin up a 2nd scriptworker daemon for dep. I'm ok with either.

escapewindow
escapewindow previously approved these changes Dec 9, 2020
Copy link
Contributor

@escapewindow escapewindow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Let's have a discussion about level1 vs 3. The schema / format-specific-config doesn't have to block.

taskcluster/ci/config.yml Show resolved Hide resolved
@@ -59,6 +62,12 @@ def build_signing_task(config, tasks):
"formats": manifest["signing-formats"],
}
]
if "mac-behavior" in manifest:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these work, but ideally we'd have some sort of warning that if we're running macapp, we need to define some of these. And if we're not running macapp signing, none of these are valid. We could do this in a number of ways:

  • define a schema for macapp vs non-macapp
  • put these options in a format-specific dictionary that we define schemas for for each signing format
  • put these three if statements under an if format_ == "macapp": block, or similar

We don't necessarily need to block on this but we may want to track the issue at least.

taskcluster/ci/config.yml Show resolved Hide resolved
@escapewindow
Copy link
Contributor

Oops, I thought I requested changes.

@escapewindow escapewindow self-requested a review December 9, 2020 23:17
@escapewindow escapewindow dismissed their stale review December 9, 2020 23:17

requesting changes

Copy link
Contributor

@escapewindow escapewindow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with by-level back in the worker block. Thanks!

@bhearsum bhearsum merged commit e656e73 into mozilla-releng:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants