-
Notifications
You must be signed in to change notification settings - Fork 56
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
Added Cloudevents V0.3 and V1 implementations #22
Conversation
slinkydeveloper
commented
Jan 28, 2020
- added v0.3 model
- added v1 model
- parametrized the tests to check the v0.2, v0.3 and v1 spec marshalling/unmarshalling features
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@denismakogon The Circle CI is blocked because of a non-code reason. Please could you look at this and merge? |
Now that CI is moved from CircleCI to GitHub Actions, I think this PR needs to be rebased to pass the builds? |
Looks like just linting errors are failing. However, I'm not sure we should provide a V1 implementation here just yet given some of the issues with the API I mentioned in #26 I'd be happy to merge just the V0.3 implementation and make an 0.3.0 release until we figure out what to do with the API. @slinkydeveloper does that sound OK to you? |
Alternatively, we could just merge this and make a 1.0.0 pre-release with the caveat that the API may change when 1.0.0 is actually published. |
I definitely agree with the API concerns in #26, but that shouldn't block us merging 1.0 support in the meantime. Supporting the 1.0 spec event format is not the same thing as publishing the API for this library at 1.0, surely? |
@di the linter is happy! This one is ready to go! |
Well, right now the major.minor version of the library does correspond to the major.minor version of the spec it supports. I'm not sure if this was intentional or not, but it'd be nice to maintain that correlation: support for the v1.0 spec would land in Obviously this isn't a requirement, but it does make it a lot easier for developers to reason about the mapping between supported spec versions and library versions in the future. |
@di fyi other sdks doesn't support this convention |
That seems a small benefit for the cost of not being able to support CE 1.0 until we have an API we are happy to call 1.0. It also means we can't use SemVer (if that's desirable), because library 2.0.0 (major version bump) tells me I must make changes in my code to upgrade to this, even if the only change in the library was adding the v2 classes to the existing API, which in SemVer would be a minor version bump. Personally, I find SemVer makes it much easier to reason about library versions, but that's up to the project to decide. Also, in this scheme, how do you reason about deprecation of old CE versions? Or patch versions of the spec? Bug fixes in the Python code causing new releases? API refactorings? These things will all collide very easily without care and mangement, and it seems an uphill effort. |
OK, I'm happy with using SemVer as well. I added a changelog that makes this more explicit in #30. If that looks good, we can merge that, add a changelog entry for this PR, merge this, and make a new |
LGTM |
Signed-off-by: Dustin Ingram <di@users.noreply.github.com>
* Added v1 and v03 specs Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Added v1 and v03 specs implementations Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * linter Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * linter 2 Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Add changelog entry Signed-off-by: Dustin Ingram <di@users.noreply.github.com> Co-authored-by: Dustin Ingram <di@users.noreply.github.com> Signed-off-by: Curtis Mason <cumason@google.com>