search

Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
109 stars 166 forks source link

Breaking change detection improperly reports success due to obsolete `private preview` special case handling #7697

Closed konrad-jamrozik closed 2 days ago

konrad-jamrozik commented 5 months ago

Details in this Teams thread.

In this PR:

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:

mikekistler commented 5 months ago

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 commented 4 months ago

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

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:

image

I assume this is what we want, for now.

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

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:

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?

Question 4: discrepancy with ARM wiki

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

says:

  1. 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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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)

konrad-jamrozik commented 2 days ago

Closing. I rewrote the relevant logic and the code no longer has any special case handling as mentioned in this issue. All previews, public or private, are treated as public. As such, same-version preview changes are forbidden. Here is the logic showing that severity remains Error if it was Error and here is the rule map with their severities.