-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update testsuiterunqueued.json to replace url with uri #162
base: main
Are you sure you want to change the base?
Conversation
The testsuiterunqueued schema uses the field url under testsuite. Within the other testsuite run schemas, uri is used. This is also used in the markdown documents. This change updates the testsuiterunqueued schema to bring it in line with the other testsuite's This is in relation to: cdevents#160 Signed-off-by: Owen Bower Adams <[email protected]>
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.
thanks for noticing!
No worries! I ended up doing an implementation based on the markdown docs and examples which I then used the schema to validate, just highlighted a couple of small gotchas as a result of coming at it cold |
@@ -111,7 +111,7 @@ | |||
"name": { | |||
"type": "string" | |||
}, | |||
"url": { | |||
"uri": { |
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.
Any change to an event schema would need a stepped version of the event. And this is a non-backwards compatible change, so it would require stepping the major version of the event schema.
This might sound picky, since we're not at 1.0 of the spec yet. There might not be any implementations of this schema in production yet, but we cannot really know that, so we'd need to update this the formal way.
Reference: https://cdevents.dev/docs/primer/#versioning-of-cdevents
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 am completely onboard with being consistent in the approach, regardless of if it's pre 1.x or not, so I appreciate the comment. I don't think it's picky at all!
I'll look to amend the PR once I've got access to a laptop and will reach out with any questions.
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.
@e-backmark-ericsson I think I've updated the version correctly ( 0.4.0-draft ). As this is the first time I've dealt with the version side of things would you be able to review. Thanks!
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.
Just noticed I'll need to update the examples, will look to do that later today
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.
Thanks @obowersa!
We version events the the overall spec independently - see https://cdevents.dev/docs/primer/#versioning-of-cdevents. The spec version is so that SDKs/Users may implement a specific version of the spec.
The event version lets users know if anything changed in a specific event when adopting a new spec version.
The event version was 0.1.0
so a new minor version would be 0.2.0
and a new major version would be 1.0.0
.
There is also a small tool that you can use that will increment the version numbers for you: https://github.com/cdevents/spec/blob/main/tools/event-version.sh#L33C1-L34C1
Adding a new uri
field is a backward compatible change, but removing url
is not. Given that we are on 0.x
still, we could just remove url
and set the version to v0.2.0-draft
. If we want to be extra nice we could deprecate url
. We've never deprecated a field before, so we'll need to see if there is a way we can express that in jsonschema that we could use when generating the SDK.
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.
Thanks for the feedback @afrittoli , and the per event versioning vs the spec version makes total sense. I'd got myself a bit tripped over on that one.
I'll update the PR with the v0.2.0-draft with the current changes for now. The field deprecitation is a great question and something which I imagine will come up again in the future
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.
@olensmar since the test kube team has implemented this event, what do you think about this change?
Signed-off-by: Owen Bower Adams <[email protected]>
4a6d59e
to
a2fa0c7
Compare
Signed-off-by: Owen Bower Adams <[email protected]>
@obowersa could you rebase this PR to the latest main? |
@obowersa - |
Changes
The testsuiterunqueued schema uses the field url under testsuite. Within the other testsuite run schemas, uri is used. This is also used in the markdown documents.
This change updates the testsuiterunqueued schema to bring it in line with the other testsuite's
This is in relation to: #160
Submitter Checklist
As the author of this PR, please check off the items in this checklist: