-
Notifications
You must be signed in to change notification settings - Fork 6
Core CDEvents to render as CloudEvent and schema validation #41
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
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.
Thank you!! This looks good.
Do we have anything to remove from the old implementation?
Thank you. |
It would be nice to have it in this PR, I think, or do you plan on removing the old implementation all the end at once? |
Yes, we are still creating PRs for Source Code, Continues Integration and Deployment events to implement with this approach and planning to complete in couple of days. |
@@ -22,7 +22,7 @@ void createPipelineRunFinishedEventAsCloudEvent() { | |||
cdEvent.setSource(URI.create("http://dev.cdevents")); | |||
|
|||
cdEvent.setSubjectId("/dev/pipeline/run/subject"); | |||
cdEvent.setSubjectSource(URI.create("/dev/pipeline/run/subject")); | |||
cdEvent.setSubjectSource(URI.create("http://dev/pipeline/run/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.
NIT: why http://
? It's fine, but even the string starting with /dev
should be a valid 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.
Updated to start with /dev
Thank you! |
Ok, so we don't need to maintain a v0.1.0 version for Java-SDK and I can update this and other event PRs to use v0.1.2 from now? |
Indeed, please use v0.1.2 - there are a few bug fixes that's good to have in. |
Thank you @afrittoli, updated this PR to use 0.1.2 specification. |
@afrittoli are we good to merge this PR now. |
🎉 This issue has been resolved in |
This PR is a continuation of #35, to implement other Core events