-
-
Notifications
You must be signed in to change notification settings - Fork 13
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: include version 2.1 and new bindings #85
Conversation
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup 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.
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.
Please refer to my comments, overall is good :)
However I have problem which should be addressed in the next PR. Why do we use the names of the folder with spaces? I don't think so it's a good, because as I remember we have in plan to publish the tck
as separate package to the npm.
The another problem. Why do we have the path's format for bindings: bindings/{OBJECT...}/{BINDING}/{VERSION}
? I think that bindings/{BINDING}/{VERSION}/{OBJECT...}
will be better.
tests/asyncapi-2.1/Security Scheme Object/scramSha256/valid.yaml
Outdated
Show resolved
Hide resolved
tests/asyncapi-2.1/Security Scheme Object/scramSha512/valid.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Maciej Urbańczyk <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>
Yea I agree, do you mind creating another issue for it?
You are right, much cleaner setup, changing 🙂 |
#86 :) |
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.
One change 😄
Co-authored-by: Maciej Urbańczyk <[email protected]>
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.
LGTM
@magicmatatjahu one thing that dawned on me, me saying the bindings do not follow the spec version is bullshit 😆 At least it is once we restructure the operation, message, and channels. Do you think this is the right approach or should bindings be moved into |
@jonaslagoni We have issue for specify spec version in the binding -> asyncapi/spec#590 so I think that binding are "independent" from the spec version. I mean that we should test the binding with asyncapi version separately, so |
Description
This PR introduces a few changes to the documents.
tests/bindings/Message Bindings Object/http/valid.yaml
the path given istests/bindings/Message Bindings Object/http/0.1.0/valid.yaml
.Related issue(s)
fixes #80