Repo optional in branch and change schemas #86
Conversation
The repository is an optional field in branch and change events, both in the specification as well as in the SDK. This is because the source includes the repository information already. While there is value in specifying the repository reference separated from the SCM system URL, it should not be mandatory to do so. The schema files for branch and change events specify the repository as a mandatory field. This was an oversight, which is fixed here. Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
The schema for branch and change events was updated to make the repository non-mandatory. Update the event version in the spec. Update the valid spec version in the schema, so that events 0.1.1 are only available in spec 0.1.1.
| @@ -1,15 +1,16 @@ | |||
| { | |||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |||
| "$id": "https://cdevents.dev/0.1.0/schema/artifact-published-event", | |||
| "$id": "https://cdevents.dev/0.1.1/schema/artifact-published-event", | |||
There was a problem hiding this comment.
I didn't anticipate that when updating the spec version only, we'd need to update each individual event schema. I believe this might be cumbersome for the future, but I don't have a good alternative solution at the moment.
| "properties": { | ||
| "context": { | ||
| "properties": { | ||
| "version": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "0.1.0" | ||
| "0.1.0", | ||
| "0.1.1" |
There was a problem hiding this comment.
The contents of the schema is the same as for event version 0.1.0, so do we really need to step the event schema version here? I believe the issue is that we include the actual spec version (see comment above), and then we need to step the event schema version as well, but... Don't have a good proposal on how to deal with this at the moment. Would have been better to not include the spec version in each event schema.
There was a problem hiding this comment.
This is not the event schema version, it's the spec version.
The enum tells which versions of the spec this specific event can be used in.
Since we add a new version where this event is untouched, we need to add the new version to the list.
There was a problem hiding this comment.
Ah, you're right. I'm still a bit confused of our intended use of event version vs schema versions. I don't like that the individual event schemas state in what spec versions they are valid. I feel that that version mapping should be done on spec level only, but we can't delay this PR due to that confusion so I'll approve it as it is implemented now.
| ], | ||
| "default": "0.1.0" | ||
| "default": "0.1.1" |
There was a problem hiding this comment.
See above, since there is no change to the rest of the content, this should be kept as 0.1.0. But since the file is updated, we'd need to step this anyway.
With this versioning structure, all events will always have the same individual version as the spec version itself.
There was a problem hiding this comment.
See above, this is not the event version.
The event version is only defined in the spec right now.
I am planning after this release to add an enum to the type field as well, which will then contain the event version. That will not change unless the event is changed.
There was a problem hiding this comment.
I approve this change now, with the comment that I'd like us to re-evaluate our versioning schemes before our next major pre-release of CDEvents (as seen in issue #87)
| "properties": { | ||
| "context": { | ||
| "properties": { | ||
| "version": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "0.1.0" | ||
| "0.1.0", | ||
| "0.1.1" |
There was a problem hiding this comment.
Ah, you're right. I'm still a bit confused of our intended use of event version vs schema versions. I don't like that the individual event schemas state in what spec versions they are valid. I feel that that version mapping should be done on spec level only, but we can't delay this PR due to that confusion so I'll approve it as it is implemented now.
| ], | ||
| "default": "0.1.0" | ||
| "default": "0.1.1" |
Changes
The repository is an optional field in branch and change events, both in the specification as well as in the SDK.
This is because the source includes the repository information already. While there is value in specifying the repository reference separated from the SCM system URL, it should not be mandatory to do so.
The schema files for branch and change events specify the repository as a mandatory field. This was an oversight, which is fixed here.
Signed-off-by: Andrea Frittoli andrea.frittoli@gmail.com
Submitter Checklist
As the author of this PR, please check off the items in this checklist: