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.

Sign up for GitHub

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

Jump to bottom

feat: make channels optional #682

Closed
wants to merge 1 commit into from
Closed

feat: make channels optional #682

wants to merge 1 commit into from

Conversation

smoya
Copy link
Member

@smoya smoya commented Jan 4, 2022

Description

Part of #661.

This PR removes the required prefix from the channels field at root level of the document.

Related issue(s)
#661

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 4, 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

@smoya smoya mentioned this pull request Jan 4, 2022
Closed
@smoya
Copy link
Member Author

smoya commented Jan 18, 2022

cc @derberg @fmvilas as you both are code owners.

@jonaslagoni
Copy link
Member

Is this not a breaking change? 🤔

@magicmatatjahu
Copy link
Member

@jonaslagoni

Is this not a breaking change? 🤔

If we switch optional to required then yes, but switching required -> optional won't break any AsyncAPI document.

@fmvilas
Copy link
Member

fmvilas commented Jan 18, 2022 •
edited
Loading

Is this not a breaking change? 🤔

It will not be forward compatible but it will be backward compatible. I think we should also make it clear in the spec, that we aim for backward compatibility.

fmvilas
fmvilas approved these changes Jan 18, 2022
View reviewed changes
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.

Looks good to me. Let's wait for @derberg.

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 18, 2022 •
edited
Loading

It will not be forward compatible but it will be backward compatible. I think we should also make it clear in the spec, that we aim for backward compatibility.

@fmvilas see #688

@fmvilas fmvilas mentioned this pull request Jan 18, 2022
Closed
fmvilas
fmvilas requested changes Jan 19, 2022
View reviewed changes
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.

I thought twice about it and think this one is a breaking change and should be postponed to v3.

@dalelane
Copy link
Collaborator

I thought twice about it and think this one is a breaking change and should be postponed to v3.

I'll have a look to see if there is anything else that needs to be pulled out of 2.3 release branches

@smoya
Copy link
Member Author

smoya commented Jan 19, 2022

As per asyncapi/spec-json-schemas#146 (comment), we won't include this change in 2.3.0 but rather in 3.0.0.

@smoya
Copy link
Member Author

smoya commented Jan 19, 2022

/dnm

@asyncapi-bot asyncapi-bot deleted the branch asyncapi:2022-01-release January 31, 2022 14:44
@fmvilas fmvilas mentioned this pull request Aug 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmvilas fmvilas fmvilas requested changes

@magicmatatjahu magicmatatjahu magicmatatjahu approved these changes

Assignees
No one assigned
Labels
do-not-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@smoya @jonaslagoni @magicmatatjahu @fmvilas @dalelane @asyncapi-bot