asyncapi / diff

Diff is a library that compares two AsyncAPI Documents and provides information about the differences by pointing out explicitly information like breaking changes.
Apache License 2.0
27 stars 12 forks source link

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

Closed JFCote closed 9 months ago

JFCote commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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[bot] commented 1 year ago

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

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 :heart:

jonbossonikea commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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