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!: remove duplicate schemas #533

Merged
merged 7 commits into from
May 28, 2024

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented May 13, 2024

Description
Even though we enabled the reusability of schemas in #364, the documents were never adapted, leaving a lot of unnecessary documents. This removes those and uses the common definitions.

This means there will no longer be anything called http://asyncapi.com/definitions/x.x.x/avroSchema_v1.json or http://asyncapi.com/definitions/x.x.x/openapiSchema_3_0.json but only http://asyncapi.com/common/avroSchema_v1.json and http://asyncapi.com/common/openapiSchema_3_0.json

Related issue(s)
Should have been fixed as part of #364

@jonaslagoni jonaslagoni marked this pull request as ready for review May 13, 2024 10:01
@jonaslagoni
Copy link
Member Author

Think we have to move this over to a next release branch

Pakisan
Pakisan previously approved these changes May 21, 2024
Copy link
Member

@Pakisan Pakisan left a comment

Choose a reason for hiding this comment

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

Looks valid for me

@jonaslagoni jonaslagoni changed the base branch from master to next-major May 21, 2024 14:03
@jonaslagoni jonaslagoni dismissed Pakisan’s stale review May 21, 2024 14:03

The base branch was changed.

@jonaslagoni jonaslagoni requested a review from Pakisan May 21, 2024 14:03
Pakisan
Pakisan previously approved these changes May 21, 2024
@jonaslagoni
Copy link
Member Author

@dalelane @lbroudoux because of the Kafka binding change Github need one of you to take a look when you can 🤙

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! 🚀🌔

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.

@jonaslagoni Not a good deal to have the need of serving those common schemas under the path common on the root of http://asyncapi.com. Not only because the need of adding an extra redirection but also because it is requiring a new path /common that is not so intuitive in terms of naming (what is common in relation to the AsyncAPI website?).

WDYT about http://asyncapi.com/schemas/common? This will require a move of the common dir under schemas directory.

@jonaslagoni
Copy link
Member Author

@jonaslagoni Not a good deal to have the need of serving those common schemas under the path common on the root of http://asyncapi.com. Not only because the need of adding an extra redirection but also because it is requiring a new path /common that is not so intuitive in terms of naming (what is common in relation to the AsyncAPI website?).

WDYT about http://asyncapi.com/schemas/common? This will require a move of the common dir under schemas directory.

Schemas hold the bundled versions, and think we should keep it clean like that 🤔

What about under definitions?

@smoya
Copy link
Member

smoya commented May 22, 2024

What about under definitions?

Fine with it.

@Pakisan
Copy link
Member

Pakisan commented May 22, 2024

@jonaslagoni @smoya maybe repeat structure from jasyncapi?

Current tests, which I'm writing now, in other branch, were grouped by next logic

  • models/[v2|v3]
  • schemas/[openapi|asyncapi|avro|json]
  • /schemas/asyncapi/security schemas/[v2|v3]/{security implementation}

They could be unified for all versions, but in v3 version one property was renamed for OAuth2 flow 😣

@jonaslagoni
Copy link
Member Author

@jonaslagoni @smoya maybe repeat structure from jasyncapi?

Current tests, which I'm writing now, in other branch, were grouped by next logic

* models/[v2|v3]

* schemas/[openapi|asyncapi|avro|json]

* /schemas/asyncapi/security schemas/[v2|v3]/{security implementation}

They could be unified for all versions, but in v3 version one property was renamed for OAuth2 flow 😣

@Pakisan mind opening an issue for it? 🙂 It will for sure be lost in this PR 😅

Copy link

sonarcloud bot commented May 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jonaslagoni jonaslagoni requested a review from smoya May 22, 2024 19:04
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! 🚀🌔

@jonaslagoni jonaslagoni merged commit 8026aa3 into asyncapi:next-major May 28, 2024
9 checks passed
@jonaslagoni jonaslagoni deleted the remove_duplicate_schemas branch May 28, 2024 17:42
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 7.0.0-next-major.2 🎉

The release is available on:

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.

5 participants