-
Notifications
You must be signed in to change notification settings - Fork 26
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 Key to support custom parameters #158
Conversation
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 91.16% 93.30% +2.14%
==========================================
Files 12 11 -1
Lines 1550 1658 +108
==========================================
+ Hits 1413 1547 +134
+ Misses 101 78 -23
+ Partials 36 33 -3
📣 We’re building smart automated test selection to slash your CI/CD build times. 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.
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.
good stuff 👍
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@thomas-fossati @setrofim I've added one last commit to this PR, in case you want to take another look. |
neat! |
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.
LGTM!
This PR redesigns
Key
so all non-common parameters are stored in a map instead, even the ones from known key types (i.e. EC2, OKP, and Symmetric keys). This allowsgo-cose
to support key types and parameters that are not defined in RFC8152 without having to break the module API. See #151 for more context.The Key struct now looks like this:
I've also added some helper functions to access the known and unknown key parameters:
A good amount of new test cases have been added.
@setrofim I've remove the
intOrStr
struct, as it is not necessary with the new way of decoding keys, It is a pity, because I conceptually liked it. If we ever release a new major version, we should consider using something like that to have a better type enforcement in all map labels.