update docs

[jianyexi]

fix #234

[mikekistler]

What do we mean when we say in the docs "This is considered a breaking change even in a new api-version."? Does that mean that this change will generate an error always? Do we have tests that verify this?

[jianyexi]

What do we mean when we say in the docs "This is considered a breaking change even in a new api-version."? Does that mean that this change will generate an error always? Do we have tests that verify this?

Yes, it's always a breaking change , what want to highlight it's breaking change in new api version, We don't need to verify it here, because though the breaking change result is based to the diff result , but breaking change pipeline has its own mechanism to determine in which situation the change is breaking change

[mikekistler]

I think the documentation in this repo should describe the behavior of this project. When we say "is considered a breaking change", I think this translates to "is reported as an error when there is no api-version change, and otherwise is a warning".

See the LogBreakingChange method here, which uses Category.Error or Category.Warning based on Strict, which is set in only one place here to be true when there was no version change.

If this analysis is correct, then the documentation in this project should never say:

This is considered a breaking change even in a new api-version.

To accurately describe the behavior of this project, I think Cause (which probably should be "Severity" rather than "Cause") should be one of only three statements:

1. "This is considered a breaking change." Where "breaking change" is as described above, and from a code perspective means that the change is reported using the LogBreakingChange method.
2. "This is considered an error." From a code perspective this means that the change is reported with the LogError method. I believe this only applies to VersionsReversed.
3. "This is not considered a breaking change" or "This is an allowed change". This should be used for changes reported with LogInfo such as AddingHeader, AddedPath, AddedOperation, etc.

[jianyexi]

Your analysis is right, but it's based on the old breaking change policy which means every breaking change can be allowed in a new version, but current Azure policy has been updated, it doesn't allow some changes even in a new version and some info level changes like 'addedPath' are considered as breaking change when you update an existing version, so here I want the doc can align with the new Azure Breaking change policy, currently the doc is mainly referred by breaking change pipeline , so what's important is to ensure this doc is consistent with the current Azure policy not the rule severity which defined in the code

[mikekistler]

Is the plan to change this repo to implement the new policy? If so, I think we should do the code and docs changes together in one PR.

@weshaggard What are your thoughts on this? Should we update this project to implement the "official" Azure breaking changes rules or kept the Azure policy separate (as it is now)?

[weshaggard]

Where are these breaking change rules implemented?

As for the docs it is unfortunate that we have to maintain multiple copies of the rules. Is there any good way to link to the official breaking change docs by our tools instead of needing to keep these updated?

[mikekistler]

The Azure breaking changes rules are implemented by the breaking-changes.ts pipeline script here:

https://github.com/Azure/rest-api-specs-scripts/blob/master/src/breaking-change.ts

combined with the configuration file here:

https://github.com/Azure/azure-rest-api-specs-pipeline/blob/master/config/BreakingChangeRules.yml

Effectively what this script and config file do is implement custom severities on top of the oad rules.

[jianyexi]

Is the plan to change this repo to implement the new policy? If so, I think we should do the code and docs changes together in one PR.

@weshaggard What are your thoughts on this? Should we update this project to implement the "official" Azure breaking changes rules or kept the Azure policy separate (as it is now)?

I prefer to keep this tool as a swagger diff tool just like what's the repo name indicates, and the breaking change

The Azure breaking changes rules are implemented by the breaking-changes.ts pipeline script here:

https://github.com/Azure/rest-api-specs-scripts/blob/master/src/breaking-change.ts

combined with the configuration file here:

https://github.com/Azure/azure-rest-api-specs-pipeline/blob/master/config/BreakingChangeRules.yml

Effectively what this script and config file do is implement custom severities on top of the oad rules.

Actually we moved these code into an ado repo in Devdiv called openapi-alps , the opensource code is outdated

[jianyexi]

I prefer to keep this repo as an common swagger diff tool, not breaking change tool, as this tool has external users . Also to determine a breaking change we need more context info like 'is the RP GAed ? is it in private preview?' . We can add a separate breaking change rules doc for all the automated rules in swagger repo

[mikekistler]

I prefer to keep this tool as a swagger diff tool just like what's the repo name indicates

I agree with this. It would be nice to make the severities configurable, but perhaps we should open an issue for that and ask for a community contribution.

But this does still leave open the question of where to document the real Azure breaking changes rules. How about in the azure-rest-api-specs repo?

[jianyexi]

So do you think we can remove the 'Cause' of the rule documentation in this repo so that it only describe the swagger change , and the breaking change related info will be described in the breaking change rules document in azure-rest-api-specs repo ？

[mikekistler]

I think we should change "Cause" to "Severity" and make it reflect the severity implemented in this repo as I described in this comment.

[jianyexi]

I've created a PR Azure/azure-rest-api-specs#20021 , @mikekistler I am little concern about if we add the severity like 'this is not breaking change', it will confuse the reviewer because most rules are considered as breaking change in a new api-version.

[mikekistler]

@jianyexi I don't understand your comment. Which specific rules that this project reports as "not a breaking change", i.e. reported with LogInfo, are considered a breaking change in a new api-version?

But more to the point, I think the project has clearly categorized each type of change it detects into one of three types:

1. non-breaking (reported by LogInfo)
2. breaking (reported by LogBreakingChange), or
3. error (reported by LogError)

And the documentation should state clearly for each rule. If you are concerned with the term "severity" we could use a different word like "categorization".

I'd also be fine with completely removing the "breaking change" terminology and instead say something like "reported as a warning if there is no api-version change and otherwise as an error". Maybe this is the best solution.

What do you thnk?

[jianyexi]

@jianyexi I don't understand your comment. Which specific rules that this project reports as "not a breaking change", i.e. reported with LogInfo, are considered a breaking change in a new api-version?

Yes, I prefer not reports as 'is/not a breaking change', we can have a separate doc for breaking change , I've create a sample Azure/azure-rest-api-specs#20021 , could you give it a review ?