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!: introduce split definitions #184

Merged
merged 16 commits into from
May 16, 2022
Merged

feat!: introduce split definitions #184

merged 16 commits into from
May 16, 2022

Conversation

jonaslagoni
Copy link
Sponsor Member

@jonaslagoni jonaslagoni commented Mar 18, 2022

Description
This PR introduces split definitions and bundling behavior to provide better maintainability of the JSON Schema files.

Changes:

  • Specification extension pattern property regex needs to be valid against u flag. This means that ^x-[\\w\\d\\.\\-\\_]+$ is changed to ^x-[\\w\\d.\\-_]+$.
  • Specification components pattern property regex needs to be valid against u flag. This means that ^x-[\\w\\d\\.\\-\\_]+$ is changed to ^x-[\\w\\d.\\-_]+$
  • Pattern property regex for parameter and correlationId location needs to be valid against u flag. This means that ^\\$message\\.(header|payload)\\#(\\/(([^\\/~])|(~[01]))*)* is changed to ^\\$message\\.(header|payload)#(\\/(([^\\/~])|(~[01]))*)*
  • Fixes $ref is incorrectly defined for a server.
  • All schema id's $id now follow http://asyncapi.com/definitions/<version>/<name>.json instead of <name>. The reason for this change is to follow an accurate bundling behavior from JSON Schema.

As this is a breaking change you can read the migration guide to get more information.

Related issue(s)
Fixes #120

@smoya
Copy link
Member

smoya commented Mar 21, 2022

@jonaslagoni would you mind changing the PR title to what the main commit says:

feat!: split out definitions

package.json Outdated Show resolved Hide resolved
@jonaslagoni
Copy link
Sponsor Member Author

@dalelane @derberg @fmvilas deciding on whether we should merge this will be better now rather than later (both I and @smoya need it for the next spec versions, or at least would be preferred). Anything you see as being blocked in order to merge this?

@derberg
Copy link
Member

derberg commented Mar 23, 2022

@jonaslagoni I'm ok for 3.0 release. I assume it is about splitting definitions. Just please provide more accurate title that will be used in commit and then in release notes. Also make it clear in description, what change is released and what breaking changes are actually expected

@jonaslagoni
Copy link
Sponsor Member Author

@jonaslagoni I'm ok for 3.0 release. I assume it is about splitting definitions. Just please provide more accurate title that will be used in commit and then in release notes. Also make it clear in description, what change is released and what breaking changes are actually expected

Will do that 👍

I am gonna resolve these conflicts as well, might require one additional PR to fix though.

@jonaslagoni
Copy link
Sponsor Member Author

PR is up that updates next-major with master #187

@jonaslagoni jonaslagoni changed the title feat!: merge next-major feat!: introduce split definitions Mar 23, 2022
@jonaslagoni
Copy link
Sponsor Member Author

Added a better description of the changes, let me know if anything else have to change 👍

@derberg
Copy link
Member

derberg commented Mar 23, 2022

@jonaslagoni looks perfect now, now you just need to solve conflicts

also, can you specify what is next plan, do we just update new 3.0 in parser just like that? no chances are needed? Did you try it already with the release candidate?

@jonaslagoni
Copy link
Sponsor Member Author

also, can you specify what is next plan, do we just update new 3.0 in parser just like that? no chances are needed? Did you try it already with the release candidate?

Changes are needed but they are simple 🙂 I will make sure to update this PR as soon as it's released: asyncapi/parser-js#423

@jonaslagoni jonaslagoni changed the base branch from master to 2022-04-release March 23, 2022 14:23
@jonaslagoni jonaslagoni changed the base branch from 2022-04-release to master March 23, 2022 14:24
@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented Mar 23, 2022

@jonaslagoni looks perfect now, now you just need to solve conflicts

@derberg I don't think I can solve those conflicts... I tried with #187 but it does not seem to apply them 🧐 And I do not have write access to the repo, so you will have to resolve them I think...

@jonaslagoni
Copy link
Sponsor Member Author

Changes are needed but they are simple 🙂 I will make sure to update this PR as soon as it's released: asyncapi/parser-js#423

Or are they 🧐 Thought i did try to use the change, but apparently I did not. Looking into it.

@derberg
Copy link
Member

derberg commented Mar 23, 2022

you need to fork the repo, create a branch out from next-major, make sure it is up to date, also make sure your master is up to date. Them merge master to next-major in your fork, solve conflicts and then open a PR from your fork to upstream next-major. Makes sense?

@jonaslagoni
Copy link
Sponsor Member Author

you need to fork the repo, create a branch out from next-major, make sure it is up to date, also make sure your master is up to date. Them merge master to next-major in your fork, solve conflicts and then open a PR from your fork to upstream next-major. Makes sense?

That's what I did in #187

@derberg
Copy link
Member

derberg commented Mar 23, 2022

please double-check, as the PR you link to doesn't show any changes to package.json while here we have conflicts with it

I recommend fresh fork, as something in your current fork might not be up to date with latest master, so you do not have last commit from Sergio from last week

@jonaslagoni jonaslagoni changed the base branch from master to 2022-04-release March 24, 2022 14:12
@jonaslagoni jonaslagoni changed the base branch from 2022-04-release to master March 24, 2022 14:12
@smoya
Copy link
Member

smoya commented Apr 26, 2022

Merge! 😂 You don't even know how much I'm looking forward to these changes (I even started replacing by webpack.config.js your actual released version) :D

As mentioned by @derberg, the release is gonna happen this week, so you won't need to wait so much more for having this PR merged and released. Does it sounds reasonable?

@Dzixxx
Copy link

Dzixxx commented Apr 26, 2022

Sounds great, srsly! <3

@jonaslagoni
Copy link
Sponsor Member Author

Gonna go through the changes in 2.4 and create a pull request so we can finish this 🤞

@jonaslagoni
Copy link
Sponsor Member Author

PR is up: #212

@jonaslagoni
Copy link
Sponsor Member Author

@derberg @fmvilas @dalelane
Blocker to finish this PR: #212

@jonaslagoni
Copy link
Sponsor Member Author

The last conflict does not seem like I will be able to solve.

But I dont think there is anything else that need to happen for this to be ready.

@derberg
Copy link
Member

derberg commented May 10, 2022

Any help needed? what last conflict you have in mind that can't be solved. We won't be able to merge without solving conflicts 😄

@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented May 16, 2022

Any help needed? what last conflict you have in mind that can't be solved. We won't be able to merge without solving conflicts 😄

I do yes, it's the same as last time, where my PRs dont solve the conflicts 🙃 It required codeowner to manually resolve the conflicts.

@sonarcloud
Copy link

sonarcloud bot commented May 16, 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
0.0% 0.0% Duplication

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'm ok to merge and release major

@dalelane @fmvilas ?

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.

Yup 🚀

@jonaslagoni
Copy link
Sponsor Member Author

😮

Is it happening? 😆

@jonaslagoni
Copy link
Sponsor Member Author

Here goes nothing

/rtm

@asyncapi-bot asyncapi-bot merged commit e5edd3a into master May 16, 2022
@asyncapi-bot asyncapi-bot deleted the next-major branch May 16, 2022 16:55
@jonaslagoni
Copy link
Sponsor Member Author

Damn, when targeted branches are removed, remaining PRs are closed as well with no way to retarget them...

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@derberg derberg restored the next-major branch May 16, 2022 17:00
@derberg
Copy link
Member

derberg commented May 16, 2022

@jonaslagoni oh, bot removes it by default. I restored it

@jonaslagoni jonaslagoni deleted the next-major branch May 21, 2024 13:58
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.

Provide JSON Schema draft 7 file as part of schemas
7 participants