Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add googlepubsub bindings #141
feat: add googlepubsub bindings #141
Changes from all commits
5c7fd1e
885fdef
4b18405
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toproto
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 intoschemaSettings.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, forname
, its structured is by design and a GCP concept.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding
name
: I missed the point that I referred toschemaSettings.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 havequeue.name
orexchange.name
; we also have similar things inibmmq
). 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whitlockjc that sounds like a perfect use case for an extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the binding to add
topic
and to removedefinition
fromschemaSettings
per the discussion above.