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

Allow skipping Gatekeper check #100

Merged
merged 6 commits into from
Nov 11, 2016

Conversation

salomvary
Copy link
Contributor

Fixes #83. Wdyt?

Copy link
Contributor

@sethlu sethlu left a comment

Choose a reason for hiding this comment

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

Hi @salomvary thanks for opening this PR! I've been busy lately while not addressing the issues here. Here are a few suggestions from me for this PR before having it merged:

  • The minimist module we used in electron-osx-sign should convert --no-gatekeeper-check to --gatekeeper-check to false.
  • gatekeeper's spelled as getekeeper in the source code.

Would you mind reviewing the two?

@@ -150,6 +150,11 @@ Default to system default keychain.
Regex or function that signals ignoring a file before signing.
Default to `undefined`.

`no-getekeeper-check` - *Boolean*
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend naming it gatekeeper-assess where false disables Gatekeeper check.

@@ -35,6 +35,9 @@ DESCRIPTION
The keychain name.
Default to system default keychain.

--no-getekeeper-check
Copy link
Contributor

Choose a reason for hiding this comment

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

Like --pre-auto-entitlements, --no-pre-auto-entitlements, --gatekeeper-assess, --no-gatekeeper-assess may then enable/disable the feature.

@sethlu sethlu added this to the Release 0.4.2 milestone Nov 5, 2016
@salomvary
Copy link
Contributor Author

Thanks for the feedback @sethlu, I addressed your suggestions.

@@ -103,7 +103,7 @@ function verifySignApplicationAsync (opts) {
})

// Additionally test Gatekeeper acceptance for darwin platform
if (opts.platform === 'darwin') {
if (opts.platform === 'darwin' && opts['gatekeeper-assess']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it would be better to have && opts['gatekeeper-assess'] as && opts['gatekeeper-assess'] !== false so it is still by default enabled for users not using the command line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely should. Otherwise programmatic default is not equals to CLI and it is strange.

@sethlu
Copy link
Contributor

sethlu commented Nov 6, 2016

Thanks @salomvary! There are a few more suggestions I have; but after that, I believe the PR's ready to merge.

@@ -103,7 +103,7 @@ function verifySignApplicationAsync (opts) {
})

// Additionally test Gatekeeper acceptance for darwin platform
if (opts.platform === 'darwin') {
if (opts.platform === 'darwin' && opts['gatekeeper-assess']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely should. Otherwise programmatic default is not equals to CLI and it is strange.

@sethlu sethlu mentioned this pull request Nov 11, 2016
@sethlu sethlu merged commit 5197101 into electron:master Nov 11, 2016
sethlu added a commit to sethlu/electron-osx-sign that referenced this pull request Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants