Skip to content

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

Merged
merged 7 commits into from
May 3, 2023

Conversation

rjalander
Copy link
Contributor

This PR is a continuation of #35, to implement other Core events

Copy link
Contributor

@afrittoli afrittoli left a 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?

@rjalander
Copy link
Contributor Author

Thank you.
Yes, we need to remove the old implementation CDEventTypes.java and it's UT related files.

@afrittoli
Copy link
Contributor

Thank you. Yes, we need to remove the old implementation CDEventTypes.java and it's UT related files.

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?
Since this is a new package it may be ok, as long is all happen this week.

@rjalander
Copy link
Contributor Author

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.
I think I will create another PR once at the end.

@@ -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"));
Copy link
Contributor

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.

Copy link
Contributor Author

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

@afrittoli
Copy link
Contributor

Thank you!
It just came to mind that perhaps we should use the spec version v0.1.2 instead of v0.1.0 as starting point here since there were a couple of issues in the v0.1.0 version, the reason why we made the patch releases.

@rjalander
Copy link
Contributor Author

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?

@afrittoli
Copy link
Contributor

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.

@rjalander
Copy link
Contributor Author

Thank you @afrittoli, updated this PR to use 0.1.2 specification.

@rjalander
Copy link
Contributor Author

@afrittoli are we good to merge this PR now.

@afrittoli afrittoli merged commit 5d9b7b6 into cdevents:main May 3, 2023
@cdevents-bot cdevents-bot added the released Issue has been released label Jul 24, 2023
@cdevents-bot
Copy link
Collaborator

🎉 This issue has been resolved in v0.1.2 (Release Notes)

@cdevents-bot cdevents-bot added this to the v0.1.2 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants