-
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
refactor!: refactor code to support more signature envelopes #72
Conversation
Codecov Report
@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 70.33% 75.79% +5.46%
==========================================
Files 23 27 +4
Lines 1591 1669 +78
==========================================
+ Hits 1119 1265 +146
+ Misses 355 311 -44
+ Partials 117 93 -24
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Although the DCO check fails in this PR, the DCO checks are actually passed in the sub-PRs. @gokarnm We can "set DCO to pass" before merging this PR. |
56b3f7d
to
ca9f3f4
Compare
Signed-off-by: Binbin Li <[email protected]>
ca9f3f4
to
65b1b6a
Compare
Signed-off-by: Binbin Li <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
NOTE This PR will be merged by rebase merging. |
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.
I've reviewed all individual PRs of this PR, so LGTM.
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.
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.
left some minor comments, lgtm
PrivateKey() crypto.PrivateKey | ||
} | ||
|
||
// remoteSigningMethod wraps the remote signer to be a SigningMethod |
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.
nit: missing period in the end.
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.
Let's address them in other PRs.
// Specifies the Notary v2 Signing Scheme used by the signature. | ||
SigningScheme signature.SigningScheme `json:"io.cncf.notary.signingScheme"` | ||
|
||
// The time at which the signature was generated. only valid when signing scheme is `notary.x509` |
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.
same
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.
Let's address them in other PRs.
// as defined at https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.6. | ||
CertChain [][]byte `json:"x5c"` | ||
|
||
// SigningAgent used for signing |
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.
same
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.
Let's address them in other PRs.
This PR is a sum of PRs to refactor code to support more future signature envelopes like COSE.
All commits / PRs in this PR have been reviewed individually as follows.