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

docs: update FilterPolicy config to match AWS API #215

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

Gadam8
Copy link
Contributor

@Gadam8 Gadam8 commented Sep 18, 2023

Currently, filterPolicy configuration outlined in this file does not match what is outlined in the AWS documentation and API - https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-sns-subscription.html. The filterPolicy should only contain the JSON policy, so no need for the attributes property; which in itself was confusing as the policy can apply to the message attributes or body.

Further to this, this definition file was missing the filterPolicyScope which is needed to determine whether the user wishes to filter on the message attributes or the body.

Description

  • ...
  • ...
  • ...

Related issue(s)

Currently, filterPolicy configuration outlined in this file does not match what is outlined in the AWS documentation and API - https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-sns-subscription.html. The `filterPolicy` should only contain the JSON policy, so no need for the `attributes` property; which in itself was confusing as the policy can apply to the message attributes or body.

Further to this, this definition file was missing the `filterPolicyScope` which is needed to determine whether the user wishes to filter on the message attributes or the body.
@Gadam8 Gadam8 requested a review from KhudaDad414 as a code owner September 18, 2023 11:12
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@Gadam8 Gadam8 changed the title Update FilterPolicy config to match AWS API docs: Update FilterPolicy config to match AWS API Sep 18, 2023
@Gadam8 Gadam8 changed the title docs: Update FilterPolicy config to match AWS API docs: update FilterPolicy config to match AWS API Sep 18, 2023
@@ -97,6 +97,9 @@
"filterPolicy": {
"$ref": "#/definitions/filterPolicy"
},
"filterPolicyScope": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As filterPolicy and filterPolicyScope will be properties directly on the object they don't need to be $ref'd.

dpwdec
dpwdec previously approved these changes Sep 18, 2023
@dpwdec
Copy link
Collaborator

dpwdec commented Sep 18, 2023

LGTM! @KhudaDad414 @iancooper You happy with the change? 🐸

iancooper
iancooper previously approved these changes Sep 19, 2023
@iancooper
Copy link
Collaborator

@dpwdec Approved

@Gadam8 Gadam8 dismissed stale reviews from iancooper and dpwdec via 3c71e17 September 19, 2023 11:02
@VisualBean
Copy link
Collaborator

VisualBean commented Sep 20, 2023

whether the user wishes to filter on the message attributes or the body.

sounds odd to me.
The bindings is not for users to create, it for users to consume and learn more about the topic. I dont think consumer configuration should be a part of it.

Are filterpolicies set by the event producer to be used by the consumer?

@Gadam8
Copy link
Contributor Author

Gadam8 commented Sep 20, 2023

whether the user wishes to filter on the message attributes or the body.

sounds odd to me. The bindings is not for users to create, it for users to consume and learn more about the topic. I dont think consumer configuration should be a part of it.

Are filterpolicies set by the event producer to be used by the consumer?

Although the SNS subscription lives under the SNS remit it is under the control of the consumer of the topic. Filter policies are set by the consumer of the event, not the producer. It is the consumer who is deciding how they wish to consume the events sent by the producer, not the producer determining how their events should be consumed.

@VisualBean
Copy link
Collaborator

VisualBean commented Sep 20, 2023

Okay,
Does it make sense to communicate to consumers of the specification then?
I don't immediately see the value in communicating "we filter events based on this" - but maybe I'm missing something

Nevermind, it makes sense from the point of view of "we only receive events that fits this policy".

@Gadam8
Copy link
Contributor Author

Gadam8 commented Sep 22, 2023

Thanks @dpwdec and @iancooper for the approval. @KhudaDad414 can I get a review please?

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

Schemas seems to match with README.

@dpwdec dpwdec merged commit 6a6cd4c into asyncapi:master Sep 25, 2023
5 checks passed
@Gadam8 Gadam8 deleted the filterPolicyDefinition branch September 25, 2023 14:34
@derberg
Copy link
Member

derberg commented Sep 25, 2023

@dpwdec @KhudaDad414 just n info for future - that when you change structure of binding, you should update binding version too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants