-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix: remove required queues property #240
Conversation
@dpwdec The JSON Schema document sets the So perhaps it is a matter of fixing the examples instead? Please @iancooper could you help figuring out if we should keep |
@smoya Sorry for my slow reply. I think the examples are correct and there should be an option for users to configure |
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.
So maybe an example would help - when would you want to configure a channel, add the binding information, but not indicate the queue? Is this because the channel, under SNS, could be bound to something other than a queue (although is this naming, it is bound to an endpoint of some sort and we may not need to document non-async endpoints)
@iancooper I think the primary example that's currently committed covers this. The need arises if you are using point to point SQS. When using a p2p queue you configure information about that queue at the channel level (as opposed to the operation level) but then you will still want to include an operation ( Below I've reproduced the example as it is in the current specs but added the message user-signedup:
bindings:
sqs:
queue:
name: user-signedup-queue
fifoQueue: false
receiveMessageWaitTime: 4
redrivePolicy:
deadLetterQueue:
name: user-signedup-dlq
policy:
statements:
- effect : Allow
principal: *
action: Sqs:SendMessage
- effect : Allow
principal: *
action: Sqs:ReceiveMessage
deadLetterQueue:
name: user-signedup-dlq
messageRetentionPeriod: 1209600
fifoQueue: false
subscribe:
operationId: sendMessage
description: sends messages when a user has signed up
bindings:
sqs: {}
message:
$ref: #/some/message/path Below is how the JSON schema currently requires the operation to be configured, with the empty user-signedup:
bindings:
sqs:
queue:
name: user-signedup-queue
fifoQueue: false
receiveMessageWaitTime: 4
redrivePolicy:
deadLetterQueue:
name: user-signedup-dlq
policy:
statements:
- effect : Allow
principal: *
action: Sqs:SendMessage
- effect : Allow
principal: *
action: Sqs:ReceiveMessage
deadLetterQueue:
name: user-signedup-dlq
messageRetentionPeriod: 1209600
fifoQueue: false
subscribe:
operationId: sendMessage
description: sends messages when a user has signed up
bindings:
sqs:
queues: []
message:
$ref: #/some/message/path To my eyes this looks more strange and confusing. You don't really want to indicate an empty list of queues (confusing) or information about the queue which is already on the channel (duplication). I suppose it could be omitted entirely but that presents a different set issues in terms of clarity of the protocol you are using with the message. Hope this makes sense. |
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Description
On reviewing the binding it seems the
required: true
status for thequeues
property of thesqs
binding was not intended. There were examples from the beginning that show, for protocol ONLY documentation purposes thatqueues
property could be undefined.This PR fixes this requirement so that linting and validation tools that build from bindings specifications can function correctly.
Related issue(s)