Check API compatability guarantees

[stv0g]

How the check results will be shown?

The go-apidiff repo has an example in their README:

$ GO111MODULE=off go get golang.org/x/exp
$ cd $GOPATH/src/golang.org/x/exp/apidiff
$ go-apidiff --repo-path=../ 81c71964d733d2e3e2375a315c0e2fd4d162adc4 --print-compatible

golang.org/x/exp/apidiff
  Incompatible changes:
  - Report.Compatible: removed
  - Report.Incompatible: removed
  Compatible changes:
  - Change: added
  - Report.Changes: added

Isn't it better to have separated CI job to only check API compatibility?

Yeah, I thought about that as well. However, I expect that the results of apidiff could be different depending on the Go-version, GOOS, GOARCH as we have build-tags excluding certain parts of the code. Which means, we should run the apidiff checks for the same set of Go-versions, GOOS's, GOARCH's as we do for testing.

Still, we could to this in a separate workflow.. But I thought we could save some time & resources by doing it in a single workflow as we only need to setup Go once..

But I am open to change it..

[at-wat]

I think printing result in a step of the long log is too hard for notifying maintainers about the API breakage and can be overlooked in most time

[stv0g]

Let me check, but I was under the assumption that a breaking change will return a non-zero exit code and hence make the test fail.

This could only be solved by making the changes API compatible, or introducing a new major version.

[stv0g]

@at-wat I've confirmed that incompatible changes which result in exit code == 1. Hence the test will fail and maintainers are clearly aware of breaking changes.

[at-wat]

I've confirmed that incompatible changes which result in exit code == 1. Hence the test will fail and maintainers are clearly aware of breaking changes.

So, PRs with breaking changes never pass CI since PRs don't have information about major version bump after merging?

[stv0g]

So, PRs with breaking changes never pass CI since PRs don't have information about major version bump after merging?

Mhh, okay. Thats indeed currently broken. go-apidiff rightfully treats a major version bumb of the module as a new module and therefore complains that basically all types/functions of the old module have been removed.

We need to add an exception here to allow major version bumps to break the API. Detecting a v1 -> v2 bump is easy. We can just look at go.mod. However, a v0 -> v1 is difficult, as we must take the latest Git version tag into account. However, I assume that this tag does not exist yet at the time, the CI runs :(

@at-wat Do you have an idea how we could fix this? I am willing to contribute a new --allow-incompatible-change-on-major-version-bump flag to go-apidiff if we can work it out :)

But i think, that we would also be fine to either:

• Allowing this step of the workflow to fail, and just show a big warning
• Make it mandatory for repo/org owners to bypass this failing tests for major version bumps. I think it would be anyway good to have a safeguard or additional hurdle to bump the major version of a model.

[at-wat]

IMO, forcing major version bump on API breakage is better, however currently pion/webrtc repository often have breaking changes on minor/patch release. @Sean-Der what do you think of the change of the versioning policy?

[Sean-Der]

@at-wat i agree! It is bad that we break API without bumping.

I love having checks to make sure we do this right

[at-wat]

@stv0g We usually hold multiple API breaking changes and release them as a single major version bump. So, I think this should be used along with Conventional Commit and allow API breaking changes only when commit message has BREAKING CHANGE: footer or ! commit type suffix. Then, the next release which contains at least one breaking change results major version bump