-
Notifications
You must be signed in to change notification settings - Fork 34
feat: add & delete APIs for provisioning interface #357
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: joydeep049 <[email protected]>
setrofim
left a comment
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.
Note: our current K-V store design does not really allow for a reasonable implementation of GET/DELETE APIs. I think it may be worth re-visiting composite keys for the K-V store before tackling this (though that requires some more design work).
| @@ -0,0 +1,182 @@ | |||
| # GET & DELETE APIs for Veraison Provisioning Interface | |||
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.
This is not needed here. Instead the REST API docs (in the docs repo) should be updated to document the new REST API.
Internal structures generally do not need to be documented -- they are of no intrest to people wanting to use the API, and anyone who wants to know how the API is implemented can (and should) just read the actual code.
Finally, explanations of what has been changed/added as part of this work should be docunted in the commit messages for the commits that make those changes, not in some random file.
| // GetEndorsementsRequest is the request message for GetEndorsements RPC | ||
| type GetEndorsementsRequest struct { | ||
| // Optional key prefix to filter endorsements. If empty, returns all endorsements. | ||
| KeyPrefix string `json:"key_prefix,omitempty"` |
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 notion of a "key" should not be exposed in the REST API. The user of the API is not expected to know that endorsements are stored in a key-value store, let alone what the format of keys is (that is a purely internal implementation detail, and is liable to change at any time).
Instead parameters for narrowing down the results should refer to conceptual attributes of the endorsements, e.g. the attestation scheme.
Should we revisit this issue at a later time? I can take a look at the Composite Key issue first. |
Yes, I think so.
That issue needs a bit more design consideration/some discussion before it can be implemented. It has a proposal of what composite keys might look like, but it hasn't been fully thought out. And we should also consider what implications (if any) CoSERV has on the design of the K-V store... I think it may best to shelve this for now. |
Fixes #223
Local Tests Passing
cc @yogeshbdeshpande @thomas-fossati @setrofim