Skip to content
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

Merged
merged 7 commits into from
Apr 24, 2020

Conversation

slinkydeveloper
Copy link
Member

  • 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>
@rajeev
Copy link

rajeev commented Feb 11, 2020

@denismakogon The Circle CI is blocked because of a non-code reason. Please could you look at this and merge?

image

@di di mentioned this pull request Apr 3, 2020
@TBBle
Copy link

TBBle commented Apr 22, 2020

Now that CI is moved from CircleCI to GitHub Actions, I think this PR needs to be rebased to pass the builds?

@di
Copy link
Member

di commented Apr 22, 2020

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?

@di
Copy link
Member

di commented Apr 22, 2020

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.

@TBBle
Copy link

TBBle commented Apr 22, 2020

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?

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Copy link
Member Author

@di the linter is happy! This one is ready to go!

@di
Copy link
Member

di commented Apr 22, 2020

Supporting the 1.0 spec event format is not the same thing as publishing the API for this library at 1.0, surely?

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 cloudevents==1.0.0, if there's ever a v1.1 or a v2.0 spec, they would land in ==1.1.0 and ==2.0.0 respectively.

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.

@slinkydeveloper
Copy link
Member Author

@di fyi other sdks doesn't support this convention

@TBBle
Copy link

TBBle commented Apr 23, 2020

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.

@di di mentioned this pull request Apr 23, 2020
@di
Copy link
Member

di commented Apr 23, 2020

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 0.3.0 release.

@n3wscott
Copy link
Member

LGTM

di added 2 commits April 24, 2020 09:48
Signed-off-by: Dustin Ingram <di@users.noreply.github.com>
@di di merged commit bcacf33 into cloudevents:master Apr 24, 2020
cumason123 pushed a commit to cumason123/sdk-python that referenced this pull request Jun 23, 2020
* 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>
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.

5 participants