feat: add googlepubsub bindings #141
Conversation
whitlockjc
requested review from
KhudaDad414,
derberg,
fmvilas and
asyncapi-bot-eve
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
whitlockjc
changed the title
There was a problem hiding this comment.
@whitlockjc I added few comments. Also general comment is that I'm missing examples in the markdown files, we should have them there too
Side note, to have it in the spec we will also need to add googlepubsub key to the spec in the list of bindings, but this is later, I can guide you and send some examples.
@fmvilas new binding on the way, please review
|
|
||
| <a name="schema-definition-object"></a> | ||
|
|
||
| ### Schema Definition Object |
There was a problem hiding this comment.
This is quite a workaround 😄
Shouldn't we try to fix it on a spec level rather than add such workaround gates on a binding level?
There was a problem hiding this comment.
Well, it's really not. The whole purpose behind this is to describe the schema Cloud Pub/Sub uses to validate messages. While AsyncAPI does have support for this already in the messages themselves, there is metadata on the schema object from a Cloud Pub/Sub perspective that is still useful for documenting the API. Below is an example of how AsyncAPI's support for Avro and how its lack of Protobuf support would be documented:
# ...
components:
messages:
messageAvroAsJSON:
bindings:
googlepubsub:
schema:
definition: |
{
"type" : "record",
"name" : "Message",
"fields" : [
{
"name" : "message",
"type" : "string"
}
]
}
name: projects/jaydub-testing/schemas/message-avro
type: avro
contentType: application/json
name: projects/jaydub-testing/schemas/message-avro
payload:
fields:
- name: message
type: string
name: Message
type: record
schemaFormat: application/vnd.apache.avro+yaml;version=1.9.0
messageProto:
bindings:
googlepubsub:
schema:
definition: |
syntax = "proto2";
message Message {
required string message = 1;
}
name: projects/jaydub-testing/schemas/message-proto
type: protobuf
contentType: application/octet-stream
name: projects/jaydub-testing/schemas/message-proto
payload: true
# ...name is metadata that could/should be present, but I could see a case where if Protobuf were supported by AsyncAPI natively for Message payloads, maybe it wouldn't need to be present in the binding. Thoughts?
There was a problem hiding this comment.
I agree with @derberg: I think we absolutely have to avoid duplicating things in bindings when they should be defined in the message itself. Protobuf support should ideally follow the same approach as for Avro: having a dedicated schemaFormat entry for it. However due to proto syntax, we'll probably not be able to embed it in the AsyncAPI spec itself and use a $ref to external file.
The value you put in name (that should be human-readable according the spec) seems similar to the one put into schemaSettings.name at the Channel level. Is it just a duplication? Does it serve a specific purpose to have it structured like the schema name in your GCP project?
There was a problem hiding this comment.
When it comes to embedding versus using a $ref, to me it's six in one hand and half another in the other. Ultimately, we want the API consumer to know as much as they can about the API contract and having a visual representation of unsupported schema types could serve some purpose. As for duplication, I'm all for removing it but if it means inconsistency (duplication is removed for use case X but not use case Y), I personally would rather allow duplication and have consistency. Finally, for name, its structured is by design and a GCP concept.
There was a problem hiding this comment.
Yeah, agree that's really a workaround. Let's wait until we have Protobuf support at the spec level using schemaFormat.
There was a problem hiding this comment.
What do we do if/when GCP supports a new schema format? Do we disallow any sort of representation within the binding in hopes that AsyncAPI supports it? What do you think about making this optional instead of unnecessary?
There was a problem hiding this comment.
Regarding name: I missed the point that I referred to schemaSettings.name and that it was a different thing than the topic name...
That said I realize that topic technical name was directly in the channel path. However this one should be human-readable, independent of binding (in case of multi-bindings supports) and may also contain parameters!
In other bindings, we decided to add a specific property within the channel binding in order to add the technical name of the destination (look at amqp where we have queue.name or exchange.name ; we also have similar things in ibmmq). I think we should do the same here so that all the technical things related to a binding are kept into this binding.
We'd ended up with something like:
channels:
my-channel-name-with-business-meaning:
bindings:
googlepubsub:
topic: projects/your-project/topics/topic-avro-schema
schemaSettings:
encoding: avro
name: projects/your-project/schemas/message-avroThere was a problem hiding this comment.
What do we do if/when GCP supports a new schema format? Do we disallow any sort of representation within the binding in hopes that AsyncAPI supports it?
@whitlockjc that sounds like a perfect use case for an extension.
There was a problem hiding this comment.
I can go either way, but ultimately I need your agreement to get this submitted so I don't feel I've got much choice here. Requiring AsyncAPI to support a schema format formally versus allowing a binding to store potentially duplicate information might not be my first choice, the whole point of x-* is to fill the gaps so I don't care enough to push back.
There was a problem hiding this comment.
I updated the binding to add topic and to remove definition from schemaSettings per the discussion above.
|
Feedback responded to, PR updated based on resolved feedback. |
whitlockjc
force-pushed
the
master
branch
2 times, most recently
from
ec7d81c
to
a7c013c
Compare
|
I added examples to the Markdown file, and added |
|
|
||
| <a name="schema-definition-object"></a> | ||
|
|
||
| ### Schema Definition Object |
There was a problem hiding this comment.
I agree with @derberg: I think we absolutely have to avoid duplicating things in bindings when they should be defined in the message itself. Protobuf support should ideally follow the same approach as for Avro: having a dedicated schemaFormat entry for it. However due to proto syntax, we'll probably not be able to embed it in the AsyncAPI spec itself and use a $ref to external file.
The value you put in name (that should be human-readable according the spec) seems similar to the one put into schemaSettings.name at the Channel level. Is it just a duplication? Does it serve a specific purpose to have it structured like the schema name in your GCP project?
|
|
||
| <a name="schema-definition-object"></a> | ||
|
|
||
| ### Schema Definition Object |
There was a problem hiding this comment.
Yeah, agree that's really a workaround. Let's wait until we have Protobuf support at the spec level using schemaFormat.
|
|
||
| <a name="schema-definition-object"></a> | ||
|
|
||
| ### Schema Definition Object |
There was a problem hiding this comment.
Regarding name: I missed the point that I referred to schemaSettings.name and that it was a different thing than the topic name...
That said I realize that topic technical name was directly in the channel path. However this one should be human-readable, independent of binding (in case of multi-bindings supports) and may also contain parameters!
In other bindings, we decided to add a specific property within the channel binding in order to add the technical name of the destination (look at amqp where we have queue.name or exchange.name ; we also have similar things in ibmmq). I think we should do the same here so that all the technical things related to a binding are kept into this binding.
We'd ended up with something like:
channels:
my-channel-name-with-business-meaning:
bindings:
googlepubsub:
topic: projects/your-project/topics/topic-avro-schema
schemaSettings:
encoding: avro
name: projects/your-project/schemas/message-avro
|
All feedback has been addressed, please check if there are any places where I messed up the changes or if I missed anything. |
|
I crossed the streams and was using |
This commit contains two bindings, a `Channel Binding Object` and a `Message Binding Object` for Google Cloud Pub/Sub using the `googlepubsub` protocol name.
There was a problem hiding this comment.
I think it looks good now 👍🏼
sorry @whitlockjc on the pushback with schema stuff, but AsyncAPI is a mature project with a well-defined contribution guide and a stable release schedule. So I definitely encourage you to help us push forward proto support in the core spec.
once others approve and we merge, I'll explain what you need to do in the main spec to enable this binding. Unless you already know?
|
@char0n this PR can probably be included in 2.5.0. Pinging you so have it on your radar. |
|
@fmvilas yep, I'm already watching this. Thanks. |
|
LGTM 👍 Thanks @whitlockjc for the successive updates. |
No need to explain, I completely understand...although to my defense, I'm not sure where advocating for more documentation (isolated to the binding itself mind you) with the potential for duplication when AsyncAPI doesn't support a native schema format would jeopardize or question AsyncAPI being a "mature project with a well-defined contribution guide and a stable release schedule." I think proto support would be awesome, something I'd love to help with. As for the next step, I think I understand so I will get this sorted when the time comes. |
|
@whitlockjc hehe, no worries. I mean mature in the way that it is easy to improve, all is transparent, and if you drive things, you do not have to wait 2 years for the release or anything like that 😄 In the case of features, yeah 🤷🏼 we have what the community wants, and believe me or not, proto is not something people talk about all the time 😄 But I definitely agree that support for proto and other formats, like xsd would be huge for AsyncAPI |
|
We're on the same page, thank you so much for your help in getting this all sorted. I'm looking forward to working with AsyncAPI more in the future. ❤️ |
|
ok then, looks like we can merge this one. but as I wrote, merging doesn't mean it is released as part of the spec. @whitlockjc if you want it in the September window, released with 2.5.0 then:
@char0n is the release coordinator for the September release. So sync with him in asyncapi/spec#830 as he just started and needs to prepare release branches. So definitely do not open PRs before he is done preparing them 😉 all good? |
|
@derberg, given asyncapi/spec#734, there's no need for ad-hoc release branches anymore. |
|
oh, @char0n I had no idea that @smoya already created I still recommend updating these release branches with latest |
Yep, it's on my radar as next task. |
|
I'll get on these other tasks tonight/tomorrow. |
Adds support for Google Cloud Pub/Sub to the `Channel Binding Object` and `Message Binding Object` lists. See: asyncapi/bindings#141
|
All necessary PRs have been created, all referencing the others. |
Adds support for Google Cloud Pub/Sub bindings. See: asyncapi/bindings#141
Adds support for Google Cloud Pub/Sub bindings. See: asyncapi/bindings#141
|
In order to merge this, we still require code review of one additional code owner. @KhudaDad414 can you please look into this PR? Thanks! |
|
@whitlockjc I invited you to this repo as maintainer. From now on, you are a CODEOWNER for googlepubsub binding. As a maintainer in one of AsyncAPI projects you also have right to join the AsyncAPI TSC and participate in decision making process for the entire initiative. Just lemme know if you are interested to learn more. |
|
Good to go. Merging. |
|
/rtm |
This commit contains two bindings, a
Channel Binding Objectand aMessage Binding Objectfor Google Cloud Pub/Sub using thegooglepubsubprotocol name.Related issue(s)
JSON Schema PR: asyncapi/spec-json-schemas#253
Spec PR: asyncapi/spec#836