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

Question about the meaning of "breaking" and "non-breaking" #150

Closed
JFCote opened this issue May 26, 2023 · 7 comments
Closed

Question about the meaning of "breaking" and "non-breaking" #150

JFCote opened this issue May 26, 2023 · 7 comments
Labels

Comments

@JFCote
Copy link

JFCote commented May 26, 2023

Hi code owners!

@aayushmau5 @vinitshahdeo @onbit-uchenik @magicmatatjahu @derberg

I'm looking to use this project in my PRs (via the AsyncAPI CLI) to check if the contract has changed between the main spec and the branch spec that is currently in PR. When the contract is broken, the PR validation fails.
The problem right now is that it seems my definition of breaking changes is not the same as yours.

When I check the standard.ts file, I see a lot of things that are considered breaking but that do not break the contract.
A simple example of this would be changing /info/version that is considered breaking. I can see this happening pretty often when a developer see that there is a new version of AsyncAPI and update the spec to the latest version.

I also see that there are currently no validation for breaking changes for components and schemas which is, in my opinion, the main reasons for breaking changes...

Thus, I would like to know what is your definition of a breaking change? I think it might be interesting to add it to the readme too after the discussion here is done.

In my opinion, a breaking change should be anything that do not respect the contract of the "old" spec.
A service using the "new" spec that passed the test (no breaking changes) should have no impact on any clients either as a publisher or a subscriber.

This is usually by preventing the removal of fields, adding required fields, removing/renaming complete components/schemas, changing or deleting channels, etc...

When I look at the standard right now, it doesn't seem to match this definition at all so I would really like to have your feedback on what is considered a breaking change before I propose a PR.

===

P.S: If we have different "breaking" definitions, I could propose a concept for "presets" which would allow anyone to use different "standard" following a specific preset depending on its definition of a breaking change?

Let me know what you think, and I will help with a PR very soon.
Thanks

@github-actions
Copy link

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@aayushmau5
Copy link
Member

@JFCote

When I check the standard.ts file, I see a lot of things that are considered breaking but that do not break the contract.

At the time of defining the default standard file, we thought of some common properties that should be considered breaking such as changing the AsyncAPI version, protocol, etc. However, I do agree that some classification like /info/version being a breaking change does not make sense.

I also see that there are currently no validation for breaking changes for components and schemas which is, in my opinion, the main reasons for breaking changes...

Can you elaborate a bit? You can provide custom classification for components.

Currently we define a breaking change as any change to your AsyncAPI document that results in breaking of a "service". For example, if a protocol has been changed from http to mqtt(defined in the standard here: https://github.com/asyncapi/diff/blob/master/src/standard.ts#LL104C3-L104C24). But we don't really define a lot of default classifications.

When I look at the standard right now, it doesn't seem to match this definition at all

I'm curious to see how the default standard should look like according to you :)
We can then try to find some common ground for defining what is considered a breaking/non-breaking change.

@JFCote
Copy link
Author

JFCote commented Jun 5, 2023

@aayushmau5 Thank for the clarification! We are currently working on an override that will represent what we think should be a diff with our vision. Once it is done and validated on our side, we will create a PR with what we suggest and we will see from there!
Thanks!

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Oct 4, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 1, 2024
@jonbossonikea
Copy link

Hi, just wondering if any changes was made :) Just tried out this tool and I don't get breaking for things I do consider a breaking change like changing a property name (similar things as what was mentioned above). Will these kind of things be handled in the future or maybe this tool is not intended for those kind of scenarios?

Thanks :)

@JFCote
Copy link
Author

JFCote commented Sep 4, 2024

Hi @jonbossonikea ! We have been working with this tool for a while now but we created a custom file for what we consider a diff. Maybe I could create a PR with this file to suggest it replaces the main one or that we have maybe a preset in the command line to select the one that is best.
Let me check that with my team.

@jonbossonikea
Copy link

Hi @JFCote, thanks for your reply :) That would be perfect, would really like to use this in order to validate payloads as well :)

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

No branches or pull requests

3 participants