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

Update ci-fix.md: dedup breaking changes guidance #28441

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Update ci-fix.md: dedup breaking changes guidance #28441

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions documentation/ci-fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,20 +126,11 @@ Refer to [Semantic and Model Violations Reference](https://github.com/Azure/azur

## `Swagger BreakingChange` and `BreakingChange(Cross-Version)`

- An API contract is identified by its api-version value. Once published, no changes to this API contract are allowed. This applies regardless of whether the API contract is for private preview, public preview, or GA (stable).
- The same-version breaking change linter rules check for changes to an existing api-version OpenAPI spec.
- When introducing a new API contract (preview or not), the new API contract must be backwards compatible with the previous GA’s API contract.
- However, during a (private or public) preview cycle, a new preview API contract does not have to be backwards compatible with the previous preview API contract although it must still be backwards compatible with the latest GA API contract.
- The cross version breaking change linter rules checks for this by comparing the new OpenAPI spec with the latest GA OpenAPI spec. If there is no latest GA OpenAPI spec, then the latest preview if it > 1 year old. If nether a GA or preview > 1 year old exists, then the OpenAPI spec is considered good.

### Adding label on PR automatically

The breaking change check has two types of violations: one is breaking change in the same version but not breaking change in a new version, the other is breaking change even in a new version.
For the former, a label 'NewApiVersionRequired' will be added automatically; For the latter, a label 'BreakingChangeReviewRequired' will be added automatically. Adding each label will trigger a github comment with guldance on how to fix.
See [aka.ms/azsdk/pr-brch-deep](https://aka.ms/azsdk/pr-brch-deep). If you want a quick read, see only [the `summary` section](https://aka.ms/azsdk/pr-brch-deep#summary).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konrad-jamrozik @kurtzeborn is there an updated Public source for this guidance? We're seeing issues where Service Teams are regularly breaking this guidance already (e.g. #28380, from less than a week ago) so I'm concerned that removing the Public source for this is only going to exacerbate this issue unfortunately

Copy link
Author

@konrad-jamrozik konrad-jamrozik Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombuildsstuff the linked doc should be available to anyone within Microsoft. Is this not the case?

In any case, we do not rely on documentation for preventing breaking changes, but on automated checks and resulting review by Breaking Change Review Board.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konrad-jamrozik unfortunately there's a considerable number of folks interacting with this repository who don't work at Microsoft and yet need to reference that document to raise issues with Service Teams.

Whilst I appreciate that your team may not rely on this information, I've found that the breaking change detector is pretty broken (see links below) - so given that external users contribute to this repository, there needs to be a public reference for this; in addition to whatever automated tooling is used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombuildsstuff

Re:

Adding a new optional QueryString parameter isn't a breaking change: #25080 (it could be to the SDK, but this is the API definition rather than the SDK being changed)

I see:

https://github.com/Azure/azure-rest-api-specs/pull/25080/checks

image

And the PR got VersioningReviewRequired which means this PR possibly violates Azure Versioning Policy due to compatible (non-breaking) change.

In general, you made some statements about what is or what is not a breaking change, but I suspect you are unaware of the Azure Versioning Policy that dictates what is flagged by the tooling or not. This policy may not be in agreement with your assessments.

Regarding the need for docs becoming public: do you perhaps have an issue you could point me to where you elaborate on your use case? This will help my team to evaluate which docs to make public.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffreyRichter in case you are interested, this thread has more feedback from @tombuildsstuff .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI for @tombuildsstuff (and whoever else might read this)...
Azure has both versioning and breaking change policies.

ANY change to an API contract REQUIRES a new api-version value. So, adding a new optional parameter is not breaking but a new api-version is still required. Therea re multiple reasons for this policy but one reason is because a might need to know what api-version supports the new optional parameter and if the api-version doesn't change, then there is no way to know if it is supported on their specific cloud.

Azure's breaking change policy goes like this: Customers should be able to adopt a new api-version of a service without requiring ANY other code changes to get the same exact behavior they got before. If a code change is REQUIRED, then the customer is broken. Some changes will not break the HTTP API but may break SDKs - in this case, we still consider them breaking as a large set of Azure customers use our SDKs.


### Run `oad` locally

To repro issues with "breaking changes" checks, you can locally run the tool that powers them [Azure/openapi-diff](https://github.com/Azure/openapi-diff) aka `oad`:
To repro issues with "breaking changes" checks, you can locally run the tool that powers them: [Azure/openapi-diff](https://github.com/Azure/openapi-diff), aka `oad`:
```
npm install -g @azure/oad
oad compare <old-spec-path> <new-spec-path>
Expand Down Expand Up @@ -306,4 +297,4 @@ Following checks have been removed from the validation toolchain as of August 20
[aka.ms/azsdk/support/specreview-channel]: https://aka.ms/azsdk/support/specreview-channel
[aka.ms/azsdk/support]: https://aka.ms/azsdk/support
[TypeSpec Discussions Teams channel]: https://teams.microsoft.com/l/channel/19%3A906c1efbbec54dc8949ac736633e6bdf%40thread.skype/TypeSpec%20Discussion%20%F0%9F%90%AE?groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47
[APIView Support Teams Channel]: https://teams.microsoft.com/l/channel/19%3A3adeba4aa1164f1c889e148b1b3e3ddd%40thread.skype/APIView?groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47
[APIView Support Teams Channel]: https://teams.microsoft.com/l/channel/19%3A3adeba4aa1164f1c889e148b1b3e3ddd%40thread.skype/APIView?groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47