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.

Sign up for GitHub

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

Jump to bottom

Update testsuiterunqueued.json to replace url with uri #162

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+4 −4
Open

Update testsuiterunqueued.json to replace url with uri #162

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cdda32
Update testsuiterunqueued.json to replace url with uri
obowersa Aug 25, 2023
a2fa0c7
Update testsuiterunqueued.json version
obowersa Sep 7, 2023
34716b9
Update event version
obowersa Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion schemas/testsuiterunqueued.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
"name": {
"type": "string"
},
"url": {
"uri": {
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

"type": "string",
"format": "uri"
}
Expand Down