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

feat: adapt googlepubsub bindings to v3 #213

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

whitlockjc
Copy link
Collaborator

@whitlockjc whitlockjc commented Jul 27, 2023

Changes from v2 to v3:

  • Bumped the bindingVersion values to 0.2.0
  • The topic property for the Channel Binding Object is no more and is now provided via the native address property of the Channel Object
  • Protobuf support now uses the native support by providing the Protobuf definition as an inline string of the schema property within the payload property of the Message Object and the schemaFormat property within the payload property of the Message Object now has valid values for Protobuf 2 and 3
  • The type property for the Message Binding Object is no more as AsyncAPI now natively support Protobuf and this value would duplicate the schemaFormat in the payload property of the Message Object

See also #182.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remember to bump the bindingVersion, in JSON Schemas too

Changes from v2 to v3:

* Bumped the `bindingVersion` values to `0.2.0`
* The `topic` property for the Channel Binding Object is no more and is now
  provided via the native `address` property of the Channel Object
* Protobuf support now uses the native support by providing the Protobuf
  definition as an inline string of the `schema` property within the `payload`
  property of the Message Object and the `schemaFormat` property within the
  `payload` property of the Message Object now has valid values for Protobuf 2
  and 3
* The `type` property for the Message Binding Object is no more as AsyncAPI
  now natively support Protobuf and this value would duplicate the
  `schemaFormat` in the `payload` property of the Message Object
@whitlockjc
Copy link
Collaborator Author

please remember to bump the bindingVersion, in JSON Schemas too

Done: asyncapi/spec-json-schemas#419

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 👍

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that someone using the binding for an AsyncAPI v2.x doc will keep using the v0.1.0 binding, while someone using the binding for an AsyncAPI v3.x doc will need to use the v0.2.0 binding?

Apologies - this is perhaps more of a general question about updating bindings than about this specific PR, but this is the first one that has got me to think about it!

@dalelane
Copy link
Collaborator

Is the idea that someone using the binding for an AsyncAPI v2.x doc will keep using the v0.1.0 binding, while someone using the binding for an AsyncAPI v3.x doc will need to use the v0.2.0 binding?

Assuming this is true and I'm not going down a misunderstanding-tangent, should we call that out in the bindings README's?

e.g.

Version

Current version for AsyncAPI v3.x documents is 0.2.0.
Current version for AsyncAPI v2.x documents is 0.1.0.

I'm thinking that it will take time for people to migrate to v3, so things like this will help ease the path.

@whitlockjc
Copy link
Collaborator Author

@dalelane That's my understanding.

@smoya
Copy link
Member

smoya commented Aug 25, 2023

Assuming this is true and I'm not going down a misunderstanding-tangent, should we call that out in the bindings README's?

Besides that, would it make sense to bump to a major version of the binding (1.0.0) instead of a minor one so it helps noticing "there is something that might break -- please check" ?

@whitlockjc
Copy link
Collaborator Author

I just followed the same pattern the other bindings made, each of which bumped the minor version and not the major version. Were there changes I made that warrant a major version bump?

cc @derberg, @jonaslagoni

@jonaslagoni
Copy link
Member

If the binding is below v0 you can do however you like 🙂

For the other bindings, there did not seem to be any interest in marking them as stable (v1), so a minor bump was done. Even if the change is breaking, i.e. you cant just bump the binding version you use in your AsyncAPI document, you have to do some more work.

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀🌔

@smoya
Copy link
Member

smoya commented Sep 12, 2023

Please @derberg @dalelane re-review this so we can move forward 🙏 . I already +1 this.

@smoya
Copy link
Member

smoya commented Sep 13, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit ec44d90 into asyncapi:next-major-spec Sep 13, 2023
12 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants