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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for Mojave app notarization #899

Merged
merged 4 commits into from Nov 6, 2018
Merged

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Oct 24, 2018

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • [] The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:
Functionality comes from electron-notarize

We can't really add tests for this because it requires a legit apple ID and no CI system is updated to a high enough macOS / Xcode version to run tests 馃槅

// Example usage
package({
  osxSign: true,
  osxNotarize: {
    appleId: process.env.MY_APPLE_ID,
    appleIdPassword: process.env.MY_APPLE_ID_PASSWORD,
  }
})

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #899 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #899   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         641    671   +30     
=====================================
+ Hits          641    671   +30
Impacted Files Coverage 螖
common.js 100% <100%> (酶) 猬嗭笍
mac.js 100% <100%> (酶) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3366253...229c69f. Read the comment docs.

malept
malept previously requested changes Oct 24, 2018
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Even if we can't test the actual use of electron-notarize, we should be able to test things like:

  • it doesn't work when notarize is true but sign is not
  • users can't pass nested fields like appBundleId and appPath to electron-notarize

@malept
Copy link
Member

malept commented Oct 30, 2018

@MarshallOfSound this PR is dependent upon #900 due to the version of Node electron-notarize targets.

@malept
Copy link
Member

malept commented Nov 5, 2018

no CI system is updated to a high enough macOS / Xcode version to run tests 馃槅

What are the minimum macOS/Xcode versions? Travis added Xcode 10.1 recently.

@malept malept dismissed their stale review November 5, 2018 18:06

I made changes based off of my comments

@malept
Copy link
Member

malept commented Nov 5, 2018

@MarshallOfSound I added tests (and moved some code around in the process). Could you take a look to see that the tests are the expected behavior?

@MarshallOfSound
Copy link
Member Author

@malept Seems legit to me, thanks for picking that up 馃憤

@malept malept merged commit c0c8014 into master Nov 6, 2018
@malept malept deleted the notarize-support branch November 6, 2018 16:40
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