Breaking change detection improperly report success due to obsolete private preview special case handling

[konrad-jamrozik]

Details in this Teams thread.

In this PR:

• https://github.com/Azure/azure-rest-api-specs-pr/pull/16984

The Swagger BreakingChange check detected errors withing an API preview version, but didn't add any label.

The code has not reported the breaking changes review is required because it concluded it is a private preview. Source:

for (const label of rule.directive.addingLabels){
  // pr targets to rpaasmaster branch and if the based api version does exist in the main branch,
  // we can infer that this api version is in private preview.
  if (
    scenario === "SameVersion" &&
    whitelistsBranches.some(
      (b) => ctx?.targetBranch.toLowerCase() === b.toLowerCase()
    ) &&
    !ctx?.ifBaseVersionExistsInBaseBranch
  ) {
    continue;
  }

Notably, the PR modifies file at this path:

specification/hybridconnectivity/resource-manager/Microsoft.HybridConnectivity/PublicCloud/preview/2024-08-01-preview/publicCloud.json

But the main branch doesn't have the /PublicCloud subdir:
https://github.com/Azure/azure-rest-api-specs-pr/tree/main/specification/hybridconnectivity/resource-manager/Microsoft.HybridConnectivity

Hence ifBaseVersionExistsInBaseBranch is false, hence the code things it is private preview.

In natural language:

IF:

• there is a breaking change within an API version (as opposed to cross-version),
• AND the PR is targeting RPSaaSMaster or ARMCoreRPDev
• AND the modified spec is not in main branch
• THEN the automation assumes this is private preview and thus does not report breaking changes.

[mikekistler]

I don't think this logic is quite right. Azure’s versioning policy does not allow breaking changes to an existing API version, even in private preview. This is to provide users a safe migration path — the service does not just break out from underneath them.

There is special allowance for compatible changes to be made in the same API version when in private preview. This goes away when the API version moves to public preview. Once in public preview, no change to an existing API version is allowed -- all changes, even compatible ones, must be done in a new API version.

[konrad-jamrozik]

@mikekistler I opened a PR that completely removes the special casing:

• Pull Request 533615: Remove obsolete/wrong inference of private preview

Question 1: Would you say that the compatible change is conceptually equivalent to versioning issue per the confusion matrix in #7724 ?

I.e. there are two kinds of changes, breaking changes and compatible changes.

One can refer to BreakingChangeRules.yml to determine which change exactly is breaking vs compatible.

Question 2: If I understand correctly, we desire the following behavior:

IF a compatible change is made to a private preview, THEN the automation should auto-approve it.

Is that correct?

If the PR I proposed above is merged, this will not be how the code works. Instead, the automation will add VersioningReviewRequired, per the bottom row of the confusion matrix:

I assume this is what we want, for now.

However, ideally, we would like to split the case of same-version / preview into two:

• IF same-version / PUBLIC preview, THEN add VersioningReviewRequried
• IF same-version / PRIVATE preview, THEN add no labels, i.e. automatically approve

Correct? If yes, then we would need automated way of doing it, which brings me to:

Question 3: is there a way to automatically determine what API version is in private preview?

The code currently says (but won't after the PR is merged):

IF:

• there is a breaking change within an API version (as opposed to cross-version),
• AND the PR is targeting RPSaaSMaster or ARMCoreRPDev
• AND the modified spec is not in main branch
• THEN the automation assumes this is private preview and thus does not report breaking changes.

In one of the emails @JeffreyRichter wrote:

When you merge into RPSaaSMaster, you are in private preview and then breaking changes and versioning violations are detected.
Our tooling can't know if you are in private preview or public preview and therefore it always flags versioning violations. We are working on allowing the PR contributor to self-approve these but we don't have that feature working yet.

So I am wondering what is the exact definition of being in private preview, as expressed by presence of API versions in branches and if we can automatically determine it.

Specifically, are these statements true?

• IF API version is in RPSaaSMaster or ARMCoreRPDev AND NOT in main THEN it is in private preview UNLESS there are no customers yet: then it is still "in development".
• In all other cases, API version is not in private preview. It may be "in development", in public preview, or GA, but not private preview.

Question 4: discrepancy with ARM wiki

ARM wiki here:
https://eng.ms/docs/products/arm/rp_onboarding/process/onboarding#a-private-preview

says:

3. Breaking changes are ok when the service is in preview as long as the customer is informed.

which conflicts with what you wrote:

Azure’s versioning policy does not allow breaking changes to an existing API version, even in private preview. This is to provide users a safe migration path — the service does not just break out from underneath them.

Does ARM wiki need an update?

[mikekistler]

A1) Yes, a compatible change done in the same API version is a versioning issue.

A2) I don't think we should auto-approve compatible changes in the same API version. These are allowed when the API version is in private preview, but we want to allow the service team to "self-certify" that the API is in private preview and if they do that will unblock merge.

A3) Not to my knowledge, but I really wish there were.

A4) Yes, the ARM wiki should be updated.

[mikekistler]

One more thing: there are some services that are "internal only" -- none of their API versions are published in the external repo. It seems likely that the code above also allowed these to skip breaking change review. If these start getting flagged for breaking change review, we might need some special logic to exclude these.

[konrad-jamrozik]

@mikekistler @rkmanda

Pull Request 9694050: Clarify that breaking changes are not allowed in private preview. Updated file(s) from Engineering Hub for content: Azure Resource Manager (ARM)