Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Cert resolver #32

Closed
wants to merge 16 commits into from
Closed

Cert resolver #32

wants to merge 16 commits into from

Conversation

Blackbaud-BobbyEarl
Copy link

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl commented Sep 26, 2019

How to test:

Documentation PR

@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #32   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      8    +1     
  Lines         282    288    +6     
=====================================
+ Hits          282    288    +6
Impacted Files Coverage Δ
lib/utils/npm-install.js 100% <ø> (ø)
lib/help.js 100% <ø> (ø) ⬆️
lib/new.js 100% <100%> (ø) ⬆️
lib/install.js 100% <100%> (ø) ⬆️
lib/utils/clone.js 100% <100%> (ø)
index.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 91f3487...d25bfba. Read the comment docs.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Overall looks great!

lib/certs.js Outdated Show resolved Hide resolved
lib/certs.js Outdated Show resolved Hide resolved
lib/new.js Show resolved Hide resolved
@Blackbaud-BobbyEarl
Copy link
Author

Made a few changes based on your feedback @Blackbaud-SteveBrush , most notably specifying the sslRoot in one place and passing that down in to builder.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Tested on Mac and works great! Code looks good too, except for a few minor comments.

lib/certs.js Outdated Show resolved Hide resolved
lib/new.js Outdated Show resolved Hide resolved
Copy link

@Blackbaud-TerryHelems Blackbaud-TerryHelems left a comment

Choose a reason for hiding this comment

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

This PR looks fine. Left some comments in some of the successive PRs though.

topics/certs.txt Outdated
@@ -0,0 +1 @@
COMING SOON

Choose a reason for hiding this comment

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

Should we document this now or can it wait?

Choose a reason for hiding this comment

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

Added documentation.

Copy link

@Blackbaud-TerryHelems Blackbaud-TerryHelems left a comment

Choose a reason for hiding this comment

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

Looks good and tested the Windows side of things.

@Blackbaud-BobbyEarl
Copy link
Author

Closing in favor of #36

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants