Skip to content
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

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

carolynvs
Copy link
Contributor

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

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>
@carolynvs
Copy link
Contributor Author

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 parameters in a parameter set and in a credential set it has a field named credentials. Or would we have two specs? 🤷‍♀️

credentials/credentialset.go Outdated Show resolved Hide resolved
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@vdice
Copy link
Member

vdice commented Aug 17, 2020

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 vdice mentioned this pull request Aug 17, 2020
2 tasks
@carolynvs
Copy link
Contributor Author

@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

* 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>
@vdice
Copy link
Member

vdice commented Aug 20, 2020

@carolynvs Sorry for the late reply! Somehow missed the last comment. 👍 I love the alternate approach you've gone with -- great idea!

@carolynvs
Copy link
Contributor Author

@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.

@carolynvs carolynvs merged commit 4276c70 into cnabio:master Sep 30, 2020
@carolynvs carolynvs deleted the valueset-schema branch May 8, 2021 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants