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

feat: move root tags and externalDocs to the info object #794

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented May 10, 2022


title: "move root tags and externalDocs to the info object"

This PR moves tags and externalDocs (describing applications) from root to the Info Object. From an application description point of view, this makes much more sense, because all the metadata is in one place. This is more of a stylistic change, however it introduces a breaking change.

PR for spec-json-schemas repo asyncapi/spec-json-schemas#244

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

I think this makes sense.

I know I'm normally the one who is pointing out changes that would make AsyncAPI inconsistent with OpenAPI for things that aren't specific to event driven apps, but I think v3 is going to be different enough from OAI that maybe we just do what we think makes the most sense.

@magicmatatjahu
Copy link
Member Author

@dalelane Thanks for your comment! I don't think it causes incompatibility with OpenAPI, because e.g. from the beginning tags in AsyncAPI are used in a different way than in OpenAPI. In OpenAPI you define tags as objects on the root, and in operations you refer to them only by name. In AsyncAPI tags as objects can be used in all places where tags may occur. The only problem may arise if someone wants to refer an Info Object from AsyncAPI in OpenAPI, then yes, this will be a problem, because the validation should inform about the use of additional fields that are not supported (tags and externalDocs).

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Yeah makes more sense 👍

@magicmatatjahu
Copy link
Member Author

@KhudaDad414 Could you check the failing Check Markdown links job? I guess that after merging it f269234 it should be fixed, but it's not. Please correct me if I'm wrong.

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM!

As a side note, I was close to suggesting we leave the tags field at root level as well + add it to Info Object, but I think this will generate more confusion than benefit.

@jonaslagoni jonaslagoni mentioned this pull request Jul 18, 2022
@jonaslagoni
Copy link
Member

Is anything stopping us from merging this? 🤔

@smoya
Copy link
Member

smoya commented Jul 18, 2022

Is anything stopping us from merging this? 🤔

I guess the approval from the rest of the owners, e.g. @derberg @dalelane

@magicmatatjahu
Copy link
Member Author

@jonaslagoni

Is anything stopping us from merging this? 🤔

yes, failing Check Markdown links job 😄 But perhaps one of the maintainers can merge it with admin privileges.

@dalelane
Copy link
Collaborator

I'll see if we have more luck with re-running the job quickly first

@magicmatatjahu
Copy link
Member Author

I thought that problem was resolved by this PR f269234 but unfortunately no.

@dalelane
Copy link
Collaborator

I don't think the problem is actually with the links being broken - the errors are all HTTP-429 - essentially GitHub complaining about too many requests coming too quickly.

@smoya
Copy link
Member

smoya commented Jul 25, 2022

the errors are all HTTP-429

Ideally, the action would accept a header with a valid GH token, so request rate can be higher than actually needed. However there is no such option afaik. We can fix it by follow their recommendations at: https://github.com/gaurav-nelson/github-action-markdown-link-check#too-many-requests

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

LGTM

I recommend ignoring failing links. This CI check is not mandatory as still in testing phase. Pr's can be merged if it gets red. Also Khuda that is keeping an eye on it is not available at the moment. And also these errors are super strange as they never happened to us before, might be some random stuff.

@derberg
Copy link
Member

derberg commented Jul 25, 2022

/dnm

we need changes in JSON Schema and the Parser

@magicmatatjahu
Copy link
Member Author

@derberg

we need changes in JSON Schema and the Parser

I will create PR for json-schemas. For parser the current "changes" in spec is reflected in the next parserJS version - https://github.com/asyncapi/parser-js/blob/next-major/src/models/v2/info.ts#L64 You can see that it's for 2.X.X, but due to fact that we don't know how 3.0.0 finally will look like the models are only made for version 2.X.X.

@magicmatatjahu
Copy link
Member Author

Changes in json-schemas asyncapi/spec-json-schemas#244

@sonarcloud
Copy link

sonarcloud bot commented Jul 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@magicmatatjahu
Copy link
Member Author

Rebased with current next-major-spec. Could you check again? @derberg @fmvilas @dalelane Thanks!

@magicmatatjahu magicmatatjahu changed the title chore: move root tags and externalDocs to the info object feat: move root tags and externalDocs to the info object Oct 3, 2022
@magicmatatjahu magicmatatjahu force-pushed the next-major/move-tags-external-docs-to-info branch from 0a25c50 to 09270e2 Compare October 20, 2022 07:30
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@magicmatatjahu
Copy link
Member Author

I had to rebase with upstream next-major-spec. Could you check again and merge? Thanks! @derberg @fmvilas @dalelane @smoya @char0n

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Still looking good to me 👍

@fmvilas
Copy link
Member

fmvilas commented Oct 21, 2022

@magicmatatjahu do you want to merge using /rtm or want us to avoid Squash and Merge?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Oct 21, 2022

@fmvilas by /rtm but I will wait for another accepts (at least two more).

@magicmatatjahu
Copy link
Member Author

/rtm

@magicmatatjahu
Copy link
Member Author

@derberg /rtm doesn't work because you added dnm label. Could you merge it?

@asyncapi-bot asyncapi-bot merged commit f3a8468 into asyncapi:next-major-spec Oct 26, 2022
@magicmatatjahu magicmatatjahu deleted the next-major/move-tags-external-docs-to-info branch October 26, 2022 18:27
@magicmatatjahu
Copy link
Member Author

@derberg Thanks!

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major-spec.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

7 participants