Skip to content

Fix #15 by adding . (dot) between event type name and version if missing #24

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

Closed
wants to merge 3 commits into from

Conversation

zaza
Copy link
Contributor

@zaza zaza commented Apr 18, 2023

No description provided.

@zaza zaza changed the title Fix #15 by adding . (dot) between event type name and version Fix #15 by adding . (dot) between event type name and version if missing Apr 18, 2023
@zaza zaza requested a review from afrittoli April 18, 2023 12:59
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.

/approve

@zaza
Copy link
Contributor Author

zaza commented Apr 19, 2023

@afrittoli seems like the /approve trick didn't work, this PR still requires a review.

@afrittoli
Copy link
Contributor

@zaza heh, we don't have prow installed, silly me - if you rebase the PR I will merge it then

@afrittoli
Copy link
Contributor

Thanks for the update. Could you please remove the merge commit from the PR?

@zaza
Copy link
Contributor Author

zaza commented Apr 19, 2023

I'm sorry, I thought you're able to squash and merge the PR.

@afrittoli
Copy link
Contributor

I'm sorry, I thought you're able to squash and merge the PR.

No worries, probably the squash will remove the merge commit, let me try :)

@rjalander
Copy link
Contributor

@zaza @afrittoli, Currently refactoring the code the way we need to create a CDEvent as per the latest spec.
and here we used CDEvents Spec Version instead of event version as per the comment #35 (comment)

As per my knowledge will not be using https://github.com/cdevents/sdk-java/blob/main/src/main/java/dev/cdevents/CDEventTypes.java, once this PR is available with all other events #35

@afrittoli
Copy link
Contributor

@zaza @afrittoli, Currently refactoring the code the way we need to create a CDEvent as per the latest spec. and here we used CDEvents Spec Version instead of event version as per the comment #35 (comment)

As per my knowledge will not be using https://github.com/cdevents/sdk-java/blob/main/src/main/java/dev/cdevents/CDEventTypes.java, once this PR is available with all other events #35

Thanks, @zaza for this PR and @rjalander for your comment.
It sounds like we should be closing this one then - @zaza your contributions are very welcome nonetheless!
I assigned issues that are covered by #35 to @rjalander, to avoid double work on any of those.

@zaza in case you'd like to chat about the SDK and CDEvents, we have a #cdevents channel on the CDF slack, a mailing list and weekly working groups.

@zaza
Copy link
Contributor Author

zaza commented Apr 27, 2023

Closing the PR as requested.

@zaza zaza closed this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants