-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(cli): accept https.*
options
#93
Conversation
Codecov Report
@@ Coverage Diff @@
## main #93 +/- ##
=======================================
Coverage 49.93% 49.93%
=======================================
Files 10 10
Lines 1568 1568
Branches 108 108
=======================================
Hits 783 783
Misses 785 785 |
d7e303e
to
8e2314d
Compare
src/_cert.ts
Outdated
@@ -23,7 +23,7 @@ export interface CommonCertificateOptions { | |||
organization?: string; | |||
organizationalUnit?: string; | |||
emailAddress?: string; | |||
domains?: string[]; | |||
alternativeNames?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already released this, we cannot rename an option in non breaking versions. Let's keep domains
(we might also deprecate and support both)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also genrally, please try to split your pull requests into smaller ones. Like one for CLI arg, one for option rename / tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renaming is removed. When I split the PR into CLI args and CLI args test, then code coverage check shows a lesser coverage. Should I keep them split anyway?
4ae9b74
to
7b4b8e8
Compare
22a1e94
to
a541d74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for yet another nice PR! I have simplified your code a little bit, merged util into main cli, and used simple http.*
notation to map options directly. This way later we might make it even an automated feature of citty to keep code simple and expandable.
https.*
options
β Type of change
π Description
In the file
cli.ts
was a TODO. This PR extends the CLI Options, to be able to provide a certificate and key file in PEM format from the command line. The certificate and key file can be provided directly (--tlsCert cert.pem
--tlsKey key.pem
) or as a keystore (--keystore keystore.p12
. A passphrase can be passed (--passphrase pw
) and with--validity 10
and--domains "localhost, 127.0.0.1
domain names and IPs can be provided for autogeneration of certificates - while no cert and key is provided.Additionallydomains
is renamed toalternativeNames
since it's not only domains but also IPs what can be provided. The CLI options is calleddomains
since it's a bit smaller. Everything in the help will be moved a bit right otherwise. If it's --better to call italternativeNames
feel free to change it.π Checklist