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

fix: remove required queues property #240

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dpwdec
Copy link
Collaborator

@dpwdec dpwdec commented Feb 2, 2024

Description

On reviewing the binding it seems the required: true status for the queues property of the sqs binding was not intended. There were examples from the beginning that show, for protocol ONLY documentation purposes that queues property could be undefined.

publish:
  bindings:
   sqs: {}

This PR fixes this requirement so that linting and validation tools that build from bindings specifications can function correctly.

Related issue(s)

@dpwdec dpwdec changed the title Fix: Remove required queues property fix: Remove required queues property Feb 2, 2024
@dpwdec dpwdec changed the title fix: Remove required queues property fix: remove required queues property Feb 2, 2024
@smoya
Copy link
Member

smoya commented Feb 13, 2024

@dpwdec The JSON Schema document sets the queue property as required. See https://github.com/asyncapi/spec-json-schemas/blob/master/bindings/sqs/0.2.0/channel.json#L33

So perhaps it is a matter of fixing the examples instead? Please @iancooper could you help figuring out if we should keep queue field as required or not? 🙏

@dpwdec
Copy link
Collaborator Author

dpwdec commented Feb 28, 2024

@smoya Sorry for my slow reply. I think the examples are correct and there should be an option for users to configure sqs: {} to indicate the protocol for documentation purposes without being forced to provide specific infrastructure configuration.

Copy link
Collaborator

@iancooper iancooper left a 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)

@dpwdec
Copy link
Collaborator Author

dpwdec commented Mar 13, 2024

@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 (publish / subscribe) to indicate the protocol for the message and $ref to a message and schema (which only lives under the operation).

Below I've reproduced the example as it is in the current specs but added the message $ref to illustrate the need.

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 queues: [] property:

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.

Copy link

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 ❤️

@github-actions github-actions bot added the stale label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants