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

What do we define as a breaking change? #688

Closed
jonaslagoni opened this issue Jan 18, 2022 · 24 comments · Fixed by #854
Closed

What do we define as a breaking change? #688

jonaslagoni opened this issue Jan 18, 2022 · 24 comments · Fixed by #854
Assignees
Labels
area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. good first issue Hacktoberfest Label issues as available for participants of https://hacktoberfest.digitalocean.com ❔ Question A question about the spec or processes released on @next-major-spec released stale

Comments

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 18, 2022

This question emerged when @smoya wanted to make the channels field optional - #661

This change, might not be a breaking change to the spec itself, however, for tooling that relies on the channels being required and always present, it would.

So when should a change in the spec be considered a breaking change?

I see two ways we can do this (and that is assuming we want to stay with semantic releases).

Only focusing on the spec

Agreeing that as long as the spec is backward compatible, it is not a breaking change.

Agreeing what is breaking changes

Agreeing on special cases would probably be possible to minimize breaking changes in tooling and spec.

My immediate thoughts are the following that would be questionable:

Non-breaking changes:

  • Adding a property

Breaking changes:

  • Making a property required
  • Making a property optional
  • Removing a property
  • Changing the type of a property in any way

Can you think of another possible solution?

@jonaslagoni jonaslagoni added the ❔ Question A question about the spec or processes label Jan 18, 2022
@smoya
Copy link
Member

smoya commented Jan 18, 2022

You made a really good point here. I agree having in mind breaking changes also at tooling level is important.
I like the idea of treating breaking changes in different ways, depending on the context: A non-breaking change for the spec doesn't mean is not a breaking change for tooling and vice versa.

If we go in that direction, we should set obvious boundaries and consider only the tools under AsyncAPI umbrella (Code hosted on github.com/asyncapi) as implementations to check for breaking changes (unless the breaking change is clearly detected earlier).

Also, it would be nice if we build some quick tool we can run on CI to check for usages and, at least, list them on the PRs, like possible affected code. Github search is not very powerful for it.

@fmvilas
Copy link
Member

fmvilas commented Jan 18, 2022

Making a property required

@jonaslagoni making a property required is also a breaking change. If I use an AsyncAPI 2.2.0 document as the input for an AsyncAPI 2.3.0 tool, and we have made a property required in v2.3.0, it will be a breaking change. I mean, changing asyncapi: 2.2.0 to asyncapi: 2.3.0 would not be enough.

Regarding #682, if we're not sure about it, we can always postpone it to v3 now that we're working on it.

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 18, 2022

@jonaslagoni making a property required is also a breaking change. If I use an AsyncAPI 2.2.0 document as the input for an AsyncAPI 2.3.0 tool, and we have made a property required in v2.3.0, it will be a breaking change. I mean, changing asyncapi: 2.2.0 to asyncapi: 2.3.0 would not be enough.

You are right, adapted suggestion!

@boyney123
Copy link

Interesting one, as @jonaslagoni mentioned it really depends on context we talk about, spec and tools.

But I like what you mentioned @jonaslagoni , about Non-breaking changes: and Breaking changes: lists, they make sense to me.

Interesting what you mentioned @smoya about only AsyncAPI tools, I guess you have to stop somewhere, but maybe with @jonaslagoni lists and definitions then wouldn't really matter about the tools, as this kind of change would be breaking anyway.

@smoya
Copy link
Member

smoya commented Jan 19, 2022

maybe with @jonaslagoni lists and definitions then wouldn't really matter about the tools, as this kind of change would be breaking anyway.

Yeah, that's a really good point. I think we must work on defining those rules @jonaslagoni started. Specially after the conversation on asyncapi/spec-json-schemas#146 (comment)

@derberg
Copy link
Member

derberg commented Jan 24, 2022

I think this issue should end up completing as addition document in the repo, guide for codeowners to always consult with when accepting changes. But for the future, we would not only have info that for example making required optional is breaking, but also an explanation why. I for example thought that it should not be considered as breaking, but tbh, I on my own, if I would know something is required, I would not protect my code from not getting the property 🤷🏼

and I would not say what is braking for spec and what for tools. Spec without tools is useless, so it is always about breaking tools really

@github-actions
Copy link

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 May 25, 2022
@derberg derberg removed the stale label Jun 7, 2022
@derberg
Copy link
Member

derberg commented Jun 7, 2022

Contributors are welcome 🙏🏼

@github-actions
Copy link

github-actions bot commented Oct 6, 2022

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 6, 2022
@derberg
Copy link
Member

derberg commented Oct 11, 2022

I suggest we extend https://github.com/asyncapi/spec/blob/master/CONTRIBUTING.md with new section called Breaking Change vs Non Breaking Change and put it between Guiding Principles and RFC Contribution Champions.

Most of content is already provided

The specification is not much different than software. Some changes provided in the spec can cause breaking changes for tools that support it. For example, if one of the properties that was `required` becomes `optional`, it is considered a breaking change because some tools might depend on that property and fail if all suddenly it is not provided. 

_Non-breaking changes:_
- Adding a property

_Breaking changes:_
- Making a property required
- Making a property optional
- Removing a property
- Changing the type of a property in any way

Anyone is welcome to open a PR and drive it until it is merged.

/gfi docs

@derberg derberg added Hacktoberfest Label issues as available for participants of https://hacktoberfest.digitalocean.com and removed stale labels Oct 11, 2022
@asyncapi-bot
Copy link
Contributor

Hey @derberg, your message doesn't follow the requirements, you can try /help.

@derberg
Copy link
Member

derberg commented Oct 11, 2022

/gfi docs

@asyncapi-bot asyncapi-bot added area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. good first issue labels Oct 11, 2022
@Dule-martins
Copy link
Contributor

I suggest we extend https://github.com/asyncapi/spec/blob/master/CONTRIBUTING.md with new section called Breaking Change vs Non Breaking Change and put it between Guiding Principles and RFC Contribution Champions.

Most of content is already provided

The specification is not much different than software. Some changes provided in the spec can cause breaking changes for tools that support it. For example, if one of the properties that was `required` becomes `optional`, it is considered a breaking change because some tools might depend on that property and fail if all suddenly it is not provided. 

_Non-breaking changes:_
- Adding a property

_Breaking changes:_
- Making a property required
- Making a property optional
- Removing a property
- Changing the type of a property in any way

Anyone is welcome to open a PR and drive it until it is merged.

/gfi docs

I will be happy to open a PR and drive it until it is merged, but I'm kind of confused about what to do. Don't know why I'm confused 😂

@asyncapi-bot
Copy link
Contributor

Hey @Dule-martins, your message doesn't follow the requirements, you can try /help.

@Dule-martins
Copy link
Contributor

/help

@asyncapi-bot
Copy link
Contributor

Hello, @Dule-martins! 👋🏼

I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘

At the moment the following comments are supported in issues:

  • /good-first-issue {js | ts | java | go | docs | design | ci-cd} or /gfi {js | ts | java | go | docs | design | ci-cd} - label an issue as a good first issue.
    example: /gfi js or /good-first-issue ci-cd

@derberg
Copy link
Member

derberg commented Oct 17, 2022

@Dule-martins 😆 if you feel confused but you do not know why, I'm definitely not gonna be able to help you out 😆

@Dule-martins
Copy link
Contributor

@derberg now, I know my issue

I don't have clue of what to do and where to do what I need to do.

I have been reading comments for days yet still confused kindly help

@derberg
Copy link
Member

derberg commented Oct 18, 2022

@Dule-martins ok, basically ignore all comments except of #688 (comment) that summarizes what has to be done

@Dule-martins
Copy link
Contributor

Oh! Thanks, Boss ❤️️

@github-actions
Copy link

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
Copy link

github-actions bot commented Aug 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 ❤️

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.0.0-next-major-spec.18 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. good first issue Hacktoberfest Label issues as available for participants of https://hacktoberfest.digitalocean.com ❔ Question A question about the spec or processes released on @next-major-spec released stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants