Update ci-fix.md: dedup breaking changes guidance #28441
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see https://aka.ms/azsdk/specreview/merge. |
Swagger Validation Report
|
Swagger Generation Artifacts
|
|
PR validation pipeline started successfully. If there is ApiView generated, it will be updated in this comment. |
|
|
||
| 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). |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
- Bug in the Breaking Change detector: https://github.com/Azure/azure-rest-api-specs/issues/26304
- Adding a new optional QueryString parameter isn't a breaking change: authorization: adding the missing
$filteroption #25080 (it could be to the SDK, but this is the API definition rather than the SDK being changed) - Documenting an optional field that exists in the API that isn't described in the Swagger redis: adding the missing
notify-keyspace-eventsvalue #22407 - Making the URI segment parameters consistent: recoveryservicessiterecovery: fixing an inconsistency in the segment naming #21667 and make compilationJobs name segment consistent and ARM spec conformant #20588 - I'm surprised there isn't a linter for this
- Making the URI segment consistent (some are a String some are an Enum, but support both values):
EventGrid2022-06-15: refactorparentTypeinto a parameter and reference it correctly for all URI paths in the swagger #23562 - Updating the maximum value for an existing parameter: [eventhubs] Increase MaximumThroughputUnits to 40 #15619
- This one is technically a breaking change, but it's not in a practical sense: hdinsight: documenting the cluster kind #26838 (anyone using the API will be setting one of these values, therefore changing this from a String -> Enum is technically a breaking change since code would require changes - but isn't in real terms/the API behaviour)
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
@JeffreyRichter in case you are interested, this thread has more feedback from @tombuildsstuff .
There was a problem hiding this comment.
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.
Link the doc to point to the new doc as currently available in: