-
Notifications
You must be signed in to change notification settings - Fork 35
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 schemaVersion to credentialSet #229
Conversation
Add a schemaVersion field to credentialSet so that we can identify the spec version that the credential set adheres to. This was very useful for claims, we use it for data migrations when the claim spec changes, and it is best to have it _before_ we need to change the spec. Worst case we never change the spec and we all win. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
So one thing I'm unsure about is that @vdice recently added support for parameter sets. It's not in the spec, but it would be great to see that added to it and cnab-go. If we did that, would the spec still be called credentialsets? They have different schema, the list of parameters goes under a field named |
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Good questions, @carolynvs . I also noticed that an actual schema for a credentialset isn't yet present in the cnab-spec repo, beyond the in-line example/text in https://github.com/cnabio/cnab-spec/blob/master/802-credential-sets.md. We can perhaps create an issue in the spec repo to create one. I'm conflicted on whether or not to try to combine parameter set and credential set into one 'valuesource' schema or leave them separate. The latter approach perhaps has benefits in comprehension and, theoretically, flexibility in the future, if, for example, a parameter set adds or removes sources that a credential set lacks or wishes to keep... I'll create an issue in this repo around the general area of adding in parameter set definitions/logic and include the question/task around pinning to a correlating schema and/or spec tag. |
@vdice What do you think of this alternate pattern for getting at the DefaultSchemaVersion so that we don't have to check for parse errors at runtime? https://github.com/cnabio/cnab-go/pull/229/files#diff-2c1fa05be1e11946f6192fea4d31108dR20 |
f1c92ea
to
18ccfe8
Compare
* Ensure that SchemaVersion is set on new CredentialSets. * Try a new pattern for the default schema that doesn't involve checking for errors parsing the spec constant. We test the spec constant in our tests and people can rely on us having set it properly. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
18ccfe8
to
d6bf372
Compare
@carolynvs Sorry for the late reply! Somehow missed the last comment. 👍 I love the alternate approach you've gone with -- great idea! |
@radu-matei What do you think? Right now I'm writing Porter to work with 2 specs, since that is how CNAB is written today. |
Add a schemaVersion field to credentialSet so that we can identify the spec version that the credential set adheres to. This was very useful for claims, we use it for data migrations when the claim spec changes, and it is best to have it before we need to change the spec. Worst case we never change the spec again and we all win. 😄
The last change to the credential-set spec was https://github.com/cnabio/cnab-spec/releases/tag/cnab-credentialsets-1.0.0-DRAFT+b6c701f