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: add extensions from extensions catalog to schema #443

Merged
merged 16 commits into from
Mar 21, 2024
Merged

feat: add extensions from extensions catalog to schema #443

merged 16 commits into from
Mar 21, 2024

Conversation

sambhavgupta0705
Copy link
Member

@sambhavgupta0705 sambhavgupta0705 commented Nov 8, 2023

This PR is part of asyncapi mentorship program
related issue: #280

@sambhavgupta0705 sambhavgupta0705 changed the base branch from master to next-major-spec November 8, 2023 03:21
Signed-off-by: Sambhav Gupta <[email protected]>
Copy link

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
0.0% 0.0% Duplication

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@smoya
Copy link
Member

smoya commented Jan 9, 2024

@sambhavgupta0705 can you please link the issue related with this PR so people can know about it? Thanks!

@sambhavgupta0705
Copy link
Member Author

@smoya sorry for the late reply
here is the issue link #280

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.

I opened a PR to your fork with enhancements. It was more complex than I thought and in depts knowledge of JSON Schema was needed.

Before you merge my PR into your fork (that will automatically update this PR):

  • please familiarize with changes in bundler script. They were necessary if we are about to treat extensions the same as bindings
  • info schema is not an allOf because I believe best practice is already defined by bindings, where bindings have separate definition schema that accommodates all bindings for a given keyword. So this is why I suggest infoExtensions.json that will be responsible to keep a list of all info extensions, so we do not "pollute" the main info schema.

so yeah, proposal is that whenever there is a new extension in the catalog, for any part of the AsyncAPI document, like server or operations or whatever - extension should never be added inside the operations but operations should be modified into allOf and there needs to be a new operationsExtensions.json created and new schema of new extension must be listed there.

Your job now is to now add to README a section where you explain a requirement on how extension schema should be added to the repo:

  • where to put extensions:
    • what folder
    • what versioning means
    • extension name in folder structure
    • schema always in file called schema.json
  • that there needs to be a corresponding definition that relates to AsyncAPI object, like info and that it must have Extensions suffix. Basically point in readme to x-x as example

@@ -0,0 +1,15 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

the structure must change a bit, as you noticed, we initially forgot about versioning, so good you added it, but there is one more thing - each extension like in case of bindings have be independently released so:

extensions/x/0.1.0/schema.json - this is the path we should have, folder with extension name and then version and each version has given version of the schema - this is why we can have always one filename as well - schema.json

@derberg derberg changed the title feat: adding extensions to json schema feat: add extensions from extensions catalog to schema Jan 24, 2024
@derberg
Copy link
Member

derberg commented Jan 24, 2024

@sambhavgupta0705 one more thing, changes that I propose in my PR, I tested them locally after bundling schema, converting test yaml file from repo to v3 and adding below:

# yaml-language-server: $schema=/Users/wookiee/sources/asyncapi-node/schemas/3.0.0-without-$id.json
asyncapi: 3.0.0
info:
  # as you can see above locally I was using bundled local schema as yaml language, and below usage of x-x was shown as valid, and when I was using numbers I was getting errors
  x-x: MyTwitterProfile
  title: Streetlights Kafka API
  version: 1.0.0
  description: |-
    The Smartylighting Streetlights API allows you to remotely manage the city
    lights.
    ### Check out its awesome features:
  
    * Turn a specific streetlight on/off 🌃  
    * Dim a specific streetlight 😎
    * Receive real-time information about environmental lighting conditions 📈
  license:
    name: Apache 2.0
    url: 'https://www.apache.org/licenses/LICENSE-2.0'

@sambhavgupta0705
Copy link
Member Author

@sambhavgupta0705 one more thing, changes that I propose in my PR, I tested them locally after bundling schema, converting test yaml file from repo to v3 and adding below:

can you please explain this a little more whenever you get time @derberg

@derberg
Copy link
Member

derberg commented Feb 12, 2024

@sambhavgupta0705 I mean that this is how I tested to make sure my changes produce a schema that is a valid schema

please also extend this PR by adding https://github.com/asyncapi/cli/blob/master/.sonarcloud.properties file, and making sure that whatever is in tools is ignored

@derberg
Copy link
Member

derberg commented Feb 21, 2024

@sambhavgupta0705 ready for final review?

@sambhavgupta0705
Copy link
Member Author

@derberg yes it is

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sambhavgupta0705 and others added 4 commits February 27, 2024 16:28
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
@sambhavgupta0705
Copy link
Member Author

Done @derberg 🚀

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@derberg
Copy link
Member

derberg commented Feb 28, 2024

@dalelane @char0n @fmvilas @smoya

please have a look at this one - it introduces a way of adding schemas of extensions into official json schema

of course not just any random extension can be added here, but only extensions that are contributed to https://github.com/asyncapi/extensions-catalog. This should encourage community to share extensions.

and yeah, unfortunate is that the first extension we have is from old times when Twitter was Twitter, so the new extension is called x which looks strange and people will use it as x-x 😆 blame Musk

@derberg
Copy link
Member

derberg commented Mar 21, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit c7c623e into asyncapi:next-major-spec Mar 21, 2024
10 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.0.0-next-major-spec.17 🎉

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.

4 participants