-
Notifications
You must be signed in to change notification settings - Fork 26
Improve subject fields spec and schema #118
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
Conversation
Improve the spec of subject fields for all events: - link id and source to their spec definitions - add the type, linked to its spec definition - include the value of the type for each event Improve the jsonschema by constraining the subject type field to the only valid value for each event. Since all events have been patched already in v0.2-draft, there's not need to +1 the version number again within the same spec release. Fixes: cdevents#113 Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
8eaebc4 to
b863d9c
Compare
continuous-deployment.md
Outdated
| |-------|------|-------------|----------| | ||
| | id | `String` | Uniquely identifies the subject within the source. | `1234`, `maven123`, `builds/taskrun123` | | ||
| | source | `URI-Reference` | [source](../spec.md#source) from the context | `staging/tekton`, `tekton-dev-123`| | ||
| | id | `String` | See [id](../spec.md#id-subject)| `1234`, `maven123`, `builds/taskrun123` | |
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.
The spec.md links does not work, There was a change for spec.md to parent directory it seems. I think it should be:
| | id | `String` | See [id](../spec.md#id-subject)| `1234`, `maven123`, `builds/taskrun123` | | |
| | id | `String` | See [id](/spec.md#id-subject)| `1234`, `maven123`, `builds/taskrun123` | |
And also it needs to change for all spec.md links.
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.
Good catch, thank you!
I replaced all ../spec.md# with spec.md# now.
continuous-deployment.md
Outdated
| | id | `String` | Uniquely identifies the subject within the source. | `1234`, `maven123`, `builds/taskrun123` | | ||
| | source | `URI-Reference` | [source](../spec.md#source) from the context | `staging/tekton`, `tekton-dev-123`| | ||
| | id | `String` | See [id](../spec.md#id-subject)| `1234`, `maven123`, `builds/taskrun123` | | ||
| | source | `URI-Reference` | See [source](../spec.md#source-subject) | `staging/tekton`, `tekton-dev-123`| |
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.
This is not a review per se. I opened a PR #119 almost at the time with you and I am a bit confused about source. Is it from context or from subject?
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.
This table describes the fields of the subject, so the source here is that from the subject.
In many cases this will have the same value as the source from the context, which is why it's optional in here.
Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
|
@randomnoise @e-backmark-ericsson thank you for the reviews! Hopefully I addressed all comments |
Changes
Improve the spec of subject fields for all events:
Improve the jsonschema by constraining the subject type field to the only valid value for each event.
Since all events have been patched already in
v0.2-draft, there's not need to +1 the version
number again within the same spec release.
Fixes: #113
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: