-
Notifications
You must be signed in to change notification settings - Fork 8
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 ability to pass Serial Number and Common Name #15
Conversation
WalkthroughThe changes introduce new fields to the Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
testcerts.go (1)
Line range hint
166-175
: Return the error if there is an error converting the private key to a PEM block.The
NewKeyPairFromConfig
method does not return an error if there is an error converting the private key to a PEM block. Instead, it returns the error as part of theKeyPair
struct. This could lead to unexpected behavior if the error is not checked by the caller.Consider returning the error if there is an error converting the private key to a PEM block:
kp.privateKey, err = keyToPemBlock(privateKey) if err != nil { return nil, fmt.Errorf("could not convert private key to pem block: %w", err) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- kpconfig.go (2 hunks)
- testcerts.go (2 hunks)
- testcerts_test.go (4 hunks)
Additional comments not posted (5)
kpconfig.go (2)
5-5
: LGTM!The
"math/big"
import is necessary to support theSerialNumber
field of type*big.Int
. The import statement is correct.
27-28
: LGTM!The addition of the
SerialNumber
andCommonName
fields to theKeyPairConfig
struct is well-implemented:
- The field names are clear and descriptive.
- The field types are appropriate:
*big.Int
forSerialNumber
allows for optional values, andstring
is suitable forCommonName
.- The fields are correctly exported (capitalized) for access from outside the package.
- The field comments provide sufficient documentation about their purpose.
These changes enhance the functionality of the
KeyPairConfig
struct without altering existing logic or control flow, making them a safe and valuable addition to the codebase.Also applies to: 30-31
testcerts.go (1)
166-166
: LGTM!The changes to the
NewKeyPairFromConfig
method improve the functionality of the certificate generation process by allowing for more dynamic input while maintaining sensible defaults. The changes are backwards compatible and do not introduce any breaking changes.Also applies to: 170-170
testcerts_test.go (2)
Line range hint
277-337
: LGTM!The new test cases for serial number and common name look good. They cover the scenarios where these values are provided in the
KeyPairConfig
and verify that the generated certificate contains the correct values.The test cases follow the existing patterns and best practices in the file.
375-385
: Looks good!The new test case in
TestFullFlow
covers the scenario where multiple attributes are provided in theKeyPairConfig
. It follows the existing patterns and does not introduce any new logic or assertions.The addition of this test case improves the coverage of the end-to-end flow and ensures that the system behaves as expected when all relevant fields are populated.
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 the contribution! Sorry I didn’t notice this PR earlier.
Thanks for this library, we're finding it useful! 😄
Just wanted to add the ability to set a serial number and common name when creating new key pairs.
Currently we have some tests that need distinct values for these which is making it a little bit harder to test what we need.
Summary by CodeRabbit
SerialNumber
andCommonName
in certificate generation, enhancing configurability.KeyPairConfig
to validate new features and ensure correct functionality, including new test scenarios for serial numbers and common names.