-
Notifications
You must be signed in to change notification settings - Fork 11
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: implement credentialsets per spec section 802 #24
Conversation
src/cnab.rs
Outdated
|
||
/// The JSON Schemata describing the parameters | ||
/// | ||
/// TODO: Should use a suitable Rust library as the target for this. |
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.
Was the intention to fix this TODO in this PR or later? If later, probably worth associating an issue with the TODO.
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.
Created #25
src/cnab.rs
Outdated
#[serde(rename_all = "camelCase")] | ||
pub struct Output { | ||
/// An optional exhaustive list of actions producing this output | ||
pub apply_to: Option<Vec<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.
The corresponding Go type is just a slice of strings, so could we get away without Option
here and use an empty vector to represent "not present"?
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 don't have strong opinions about this one, but the Rust idiom seems to be to avoid allocating empty vectors if possible. I can go either way, though.
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.
Fair enough. Also it may be useful to distinguish between a provided empty list and the default.
Needs rebasing, but not sure why github hasn't realised. |
@glyn - most likely, GitHub didn't realise it needs a rebase because there's a branch protection setting to only allow up-to-date branches to be merged, which I just enabled. |
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
392f67d
to
d47e505
Compare
Is there something left that I need to do on this? |
This provides a minimal implementation of the credentialset format.
Depends on #23